msg.value in a loop can be used to drain proxy funds
While BoringBatchable is out of the scope, this bug affects seriously MIMOProxy as it inherits.
Some time ago I read a report about an auditor called samczsung (<https://samczsun.com/two-rights-might-make-a-wrong/>). I believe that you are having the same problem here.
I will try to explain it as brief as possible but I can add a PoC in QA stage if required.
This vulnerability comes from the fact that msg.value and msg.sender are persisted in delegatecall.
It is possible to call execute() (which is payable ) from batch() (which is also payable ) because both are public functions. (For now ignore the fact that execute() has access control).
The attacker would call batch() sending, for example, 1 ETH with an array of 100 equal items that call execute()
This execute() will call and external contract 100 times and in every time it will send 1ETH from proxy funds (not from the attacker).
If the receiving contract stores these value then the proxy wallet will be drained.
While this is already a high risk and there should be many attacking scenarios I would like to show you a pretty simple one.
Suppose the owner would like to grant access to a target with a normal function (maybe no even payable).
For example suppose that the owner grant access to the function
function goodFunction() public
This function has the selector 0x0d092393 . However, for some reason. the owner mistyped the selector and grant access to non existing function 0x0d09392.
Then if the target contract has the so common function.
fallback() external payable { }
Then the attacker can drain wallet funds using this selector as I explained above.
The solution is pretty straightforward.
Remove payable from batch() in BoringBatchable.
The text was updated successfully, but these errors were encountered:
š 1 horsefacts reacted with eyes emoji
All reactions