warRoom - Arbitary execution allows Swapper Owner to steal preapproved funds of Traders

[sherlock-admin]

warRoom

high

Arbitary execution allows Swapper Owner to steal preapproved funds of Traders

Summary

A malicious swapper owner can steal traders pre approved funds.

Vulnerability Detail

A trader needs to give approval for his funds to SwapperImpl.sol in order to use the flash() function. Later on, inside flash() function, transferFrom() is used and traders funds are transferred to the contract. Malicious swapper owner has ability to execute arbitrary transaction of his choice through WalletImpl.execCalls(). The owner of the swapper can use this function maliciously to steal the pre-approved funds of the traders by 2 ways:

1. Owner can frontrun Traders flash() transaction.
   • Trader approves the swapper contract.
   • Trader calls the flash() function.
   • But owner frontruns the flash() and calls execCalls() function with data of approved tokenToBeneficiary.transferFrom(trader, owner, amount). Note: We shouldnot assume that every trader will follow UniV3Swap.sol implementation. Traders may pre-approve early.
2. Owner can steal traders extra approved funds.
   • If any trader approves type(uint256).max trusting the swapper. In this scenario, owner does not require to frontrun as he can call transferFrom() anytime using execCalls().

Impact

• Traders funds can be stolen.

Code Snippet

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-swapper/src/SwapperImpl.sol#L273

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-utils/src/WalletImpl.sol#L43-L65

Tool used

Manual Review

Recommendation

• Swapper contract shouldn't be able to call $tokenToBeneficiary. In Swapper.execCalls(), there should be a check like below. require(calli.to != $tokenToBeneficiary);