obront - Swapper owner can frontrun callers of `flash()`, stealing funds

[sherlock-admin]

obront

medium

Swapper owner can frontrun callers of flash(), stealing funds

Summary

Users (or their bots) are expected to call flash() to capture a small fee for swapping assets into the $tokenToBeneficiary for the $beneficiary. However, by doing this, they open themselves up to a multitude of attacks that would allow the owner of a Swapper to exploit them and steal their funds.

Vulnerability Detail

At any time, a bot can call flash() to swap a Swapper's assets into a single asset for them. The bot passes in the tokens they would like to swap, and the function:

• Calculates the value of each token in the beneficiary token, and sums the value
• Sends the contract's full balance of each of these tokens to the bot
• Calls swapperFlashCallback() callback on msg.sender (the bot) to trigger them to perform the swap
• Before returning control flow, the bot either sends ETH to the contract or approves the contract to spend the beneficiary token
• The contract sends the swapped tokens (as well as any excess in the contract) to the beneficiary, ending the transaction

The problem is that, when the bot submits such a transaction to the mempool, the Swapper's owner has the opportunity to front run the transaction and change the contract's parameters to extract additional value.

This can be done in a number of ways. Two simple examples:

• They can call setOracle() on the Swapper, subbing in a fake oracle that returns higher values than it should, leading to the bot needing to overpay the owner for the tokens.
• They can increase scaledOfferFactor() far above "par", which leads to the bot needing to pay more than the value returned from the oracle for the tokens they swap.
• In times of high volatility, they can increase or decrease the po.period based on recent price movement, increasing or lowering the price.

In any of the above cases, this could result in stealing funds from the bot.

Note: I am aware that in the contest docs, it says the following: "swapper#flash callers are expected to be sophisticated (aka will check if a given txn reverts, will use flashbots rpc to avoid FR & owner-griefing, etc)". However, I felt this issue was important to submit for 3 reasons:

1) That statement is concerningly handwavy, and brushes off a large number of attacks by pushing the onus onto bot operators.

2) This audit report is not just for 0xSplits, but also for users and bot operators. If a protocol said "all our funds can be stolen but we know about it and it's not in scope", that doesn't make the issue any less severe. It's still important for this information to be included in a final report.

3) While these issues are most severe in the case of front running, they can also occur without it. For example, a Swapper might learn the patterns that bot operators use (ie execute when money hits the account) and get ahead of them that way.

Impact

Bots are put in a position to be exploited by Swapper owners.

Code Snippet

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

Trusting the oracle, which is set by the owner:

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

Trusting the scaled offer factor, which is set by the owner:

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-oracle/src/UniV3OracleImpl.sol#L284

Tool used

Manual Review

Recommendation

Make the process of changing key parameters a two step process for owners, that requires at least one block to pass in between the two calls. This ensures that no frontrunning or timing attacks are possible.

[ctf-sec]

Q: Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED? no protocol owner(s); oracle, swapper, & pass-through-wallet owners are TRUSTED