No deadline parameter in sellAllAmount() and buyAllAmount() functions:

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/511636d889742296a54392875a35e4c0c4727bb7/contracts/RubiconMarket.sol#L1028-L1112

Vulnerability details

Impact

There is no deadline parameter in these two functions,This missing feature enables pending transactions to be maliciously executed at a later point.

Proof of Concept

Consider following scnerio:

1. Alice wants to create order of 1000DAI for 1 ETH . She signs the transaction with minOutputAmount = 0.99 ETH to allow for some slippage.
   2.The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for miners to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.
2. When the average gas fee dropped far enough for Alice's transaction to become interesting again for miners to include it. In the meantime, the price of ETH could have drastically changed. She will still at least get 0.99 ETH due to minOutputAmount, but the DAI value of that output might be significantly lower. She has unknowingly performed a bad trade due to the pending transaction she forgot about.

An even worse way this issue can be maliciously exploited is through MEV:

1. The swap transaction is still pending in the mempool. Average fees are still too high for miners to be interested in it. The price of has gone up significantly since the transaction was signed (lets say its not dai now and some other token), meaning Alice would receive a lot more ETH when the swap is executed. But that also means that her minOutputAmount value is outdated and would allow for significant slippage.
2. A MEV bot detects the pending transaction. Since the outdated minOutputAmount now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Alice.

Tools Used

Manual Review

Recommended Mitigation Steps

Add deadline param

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

bghughes marked the issue as sponsor disputed

[c4-sponsor]

bghughes marked the issue as disagree with severity

[bghughes]

IMO this is a good recommendation but is OOS

[bghughes]

Due to MEV argument - seems network-level to me - nice enhancement idea though

[HickupHH3]

ignoring the MEV argument since we're dealing with L2s.

I think the first portion has some merit, referencing https://code4rena.com/reports/2022-06-canto-v2/#m-01-stableswap---deadline-do-not-work

Because Front-running is a key aspect of AMM design, deadline is a useful tool to ensure that your tx cannot be “saved for later”.

While both Arbitrum & Optimism has minimum gas prices, network congestion could mean that the tx doesn't get mined until they go back down (eg. trading during Arb airdrop).

It's again a user-conditional error, which, following the reasoning in #1298, would be med severity.

[c4-judge]

HickupHH3 marked the issue as selected for report