Anyone with valid `WithdrawRequest` can steal excess ETH from `MagicSpend`

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L181-L194

Vulnerability details

Impact

Malicious users can create non-expiring WithdrawRequest and execute it when there are funds in the MagicSpend contract due to missing _withdrawableETH check in the withdraw function.

Proof of Concept

Suppose that there is a scenario where MagicSpend has a non-zero balance - the owner calls withdrawStake or withdrawTo in EntryPoint passing the MagicSpend as a recipient, it is possible because of the exposed receive(). A malicious wallet owner can preventively issue a non-expiring signature from the contract owner and call withdraw when there are amount of tokens that do not belong to him, stealing them from other users/owner.

We can see that the _withdrawableETH mapping is supposed to indicate the right amount of funds that belong to a sender, but the above-mentioned withdraw function lacks a check and allows a user with a signed WithdrawalRequest to front-run the owner of the funds.

MagicSpend.sol#L181-L192

function withdraw(WithdrawRequest memory withdrawRequest) external {
      _validateRequest(msg.sender, withdrawRequest);

      if (!isValidWithdrawSignature(msg.sender, withdrawRequest)) {
          revert InvalidSignature();
      }

      if (block.timestamp > withdrawRequest.expiry) {
          revert Expired();
      }

      // reserve funds for gas, will credit user with difference in post op
      _withdraw(withdrawRequest.asset, msg.sender, withdrawRequest.amount);
  }

As we can see the only thing preventing attacker from executing the attack is the isValidWithdrawSignature , which will be bypassed by issuing owner’s signature in advance.

Tools Used

Manual Review

Recommended Mitigation Steps

In the receive function of the contract add the amount of received ETH to _withdrawableETH[owner()] . Then in the withdraw function use the mapping to indicate the proper withdrawable amount, also validating it against the amount argument from the signature.

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #3

[raymondfam]

See #3.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid

[blckhv]

Hey, @3docSec,

Thanks for the fast judging process, I would like to raise my concerns that this issue is wrongly duplicated with https://github.com/code-423n4/2024-03-coinbase-findings/issues/3, it says that everyone can craft a valid WithdrawSignature, whereas this reports is concerned about a valid signature, signed by the owner of the Paymaster, and intended to be used in the validatePaymasterUserOp, but instead of it is used in the withdraw when there is a clear incentive for the caller.

Thanks in advance!

[3docSec]

Hi @AydoanB, I see your point, and I believe the grouping with #3 and the judging holds.

The claim the only thing preventing attacker from executing the attack is the isValidWithdrawSignature , which will be bypassed by issuing owner’s signature in advance makes the finding inconsequential because having the owner signature is exactly what secures the withdraw function. To pass this check, one must have a signature by owner that authorizes them (not somebody else, because msg.sender is checked) to withdraw funds.

So if they steal funds, they steal theirs; if they front-run or DoS somebody, they front-run or DoS themselves, which makes the finding inconsequential,.