defsec - Persisted msg.value in a loop of delegate calls can be used to drain ETH from your proxy

[sherlock-admin]

defsec

high

Persisted msg.value in a loop of delegate calls can be used to drain ETH from your proxy

Summary

msg.value in a loop can be used to drain proxy funds.

Vulnerability Detail

While BoringBatchable is out of the scope, this bug affects seriously BasePool.sol#L17 as it inherits.

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.

Impact

msg.value in a loop can be used to drain proxy funds.

Code Snippet

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.

Tool used

Manual Review

Recommendation

Remove payable from batch() in BoringBatchable.