URLs:
While there is a check that owner is not changed in a delegatecall, such a check is missing for minGasReserve, which means that the variable can be changed (either maliciously or accidentally because of a storage slot collision). The consequences of this are severe. If the variable is set to a high value, the proxy becomes useless, as execute is no longer callable (or only with a very high gas limit) because of an underflow in the stipend calculation.
Bob, the owner of a proxy performs a delegatecall via execute to a contract that the trusts. However, this contract happens to have a variable at storage slot 2 that is set to 1,0000,000. Therefore, Bob's proxy is unusable after the execute call.
Mark the variable as immutable, as it is unchangeable anyways.
URLs:
In response to a flashloan, proxyRegistry.getCurrentProxy(owner) is used to retrieve the correct user proxy and check that this proxy has initiated the flash loan. This can be problematic after ownership transfers of the proxy, where proxyRegistry.getCurrentProxy(owner) does not reflect the current owner.
- Alice transfers her proxy to Bob through
transferOwnership - Bob now calls
executeActiononMimoRebalanceto perform a rebalance. Therefore,proxyRegistry.getCurrentProxy(address(Bob))will be called inexecuteOperation, causing the rebalance to fail.
Even worse, when Alice calls executeAction through Bob's proxy (e.g., because she gave her permissions to do so before the ownership transfer), it will succeed.
Update the proxy registry on ownership transfers.
URLs:
In deployFor, owner() is called if there is already an entry for the provided address. This can be exploited by a sophisticated attacker to make the system completely unusable for a user.
- Alice transfers her proxy to Bob through
transferOwnership - As Bob can
delegatecallarbitrary code, he calls another contract that performs aSELFDESTRUCTthroughexecute. - Now, when Alice wants to deploy a new user proxy through the registry,
currentProxy.owner()is called on her previous proxy, which reverts. Therefore, she will never be able to use the system again.
Update the proxy registry on ownership transfers.
URLs:
vaultOwner returns zero for a non-existing vaultId. Similarly, proxyRegistry.getCurrentProxy(msg.sender) returns zero when msg.sender has not deployed a proxy yet. Those two facts can be combined to set automation for a vault ID that does not exist yet. When this is done by a user without a proxy, it will succeed, as both vaultOwner and mimoProxy are address(0), i.e. we have vaultOwner == mimoProxy.
The consequences of this are quite severe. As soon as the vault is created, it will be an automated vault (with potentially very high fees). An attacker can exploit this by setting very high fees before the creation of the vault and then performing actions for the automated vault, which leads to a loss of funds for the user.
The same attack is possible for setManagement.
Do not allow setting automation parameters for non-existing vaults, i.e. check that vaultOwner != address(0).
URLs:
When the ownership of a user proxy is transferred, proxyRegistry.getCurrentProxy does not reflect this and still returns the proxy for the previous owner. This can be exploited in the access check of setAutomation.
Because setManagement has the same check, the same attack is also possible there.
- Alice transfers her user proxy to Bob.
- Bob can now call
setAutomationon a vault that is owned by Alice and set a very high fee. - Afterwards, he can for instance perform a rebalance to get the fee.
Either reflect ownership changes in proxyRegistry.getCurrentProxy or check in setAutomation that msg.sender is still the owner of the proxy.
URLs:
When the owner of a user proxy is changed, the permissions are not cleared. This can be abused to give oneself permissions and then transfer the proxy to another user.
- Alice uses
setPermissionto allow herself to call a contract that performs aSELFDESTRUCT. - Alice transfers her user proxy to Bob.
- Alice can now destroy Bob's proxy, the permissions entry is still set.
Remove all permissions on an ownership transfer.