ARB supplied through execute() function in user's ODProxy is permanently stuck

[c4-submissions]

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODProxy.sol#L30

Vulnerability details

Impact

ARB (i.e. msg.value) is supplied through the payable execute() function in the user's ODProxy but it is not forwarded further by the delegatecall, which can cause the native ARB to be permanently locked in the user's ODProxy contract.

Proof of Concept

Here is the whole process:

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODProxy.sol#L26

1. User calls the payable execute() function with the respective values and supplies msg.value amount to provide ARB as collateral.

   File: ODProxy.sol
   26:   function execute(address _target, bytes memory _data) external payable onlyOwner returns (bytes memory _response) {
   27:     if (_target == address(0)) revert TargetAddressRequired();
   28: 
   29:     bool _succeeded;
   30:     (_succeeded, _response) = _target.delegatecall(_data);
   31: 
   32:     if (!_succeeded) {
   33:       revert TargetCallFailed(_response);
   34:     }
   35:   }

2. On Line 30, we see the user's ODProxy making a delegate call on the _target address with _data but it does not forward the msg.value that was supplied by the user. This causes the funds to be stuck in the user's ODProxy contract permanently.

   File: ODProxy.sol
   29:     bool _succeeded;
   30:     (_succeeded, _response) = _target.delegatecall(_data);

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a withdrawal mechanism to withdraw this ARB.

Assessed type

call/delegatecall

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

pi0neerpat (sponsor) acknowledged

[c4-sponsor]

pi0neerpat marked the issue as disagree with severity

[c4-sponsor]

pi0neerpat (sponsor) confirmed

[pi0neerpat]

Confirmed, we may fix this. However, all collateral in the protocol, including ARB, must be ERC20. Using ARB native token via msg.value is not supported by the protocol, so its not expected to be used.

Low risk severity- user locking their own funds due to msg.value mistake is unfortunate, but not critical to the behavior of the protocol.

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

Even if the user does mistakenly transfer ETH through execute(), he can retrieve the ETH in the proxy by delegate calling into a contract that transfers ETH out. There isn't any permanent locking of funds in the proxy, as such, this finding is low severity at best.

[c4-judge]

MiloTruck marked the issue as grade-b

[c4-judge]

MiloTruck marked the issue as grade-a