Missing Deadline Checks Allow Pending Transactions to be Maliciously Executed

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0x6527a4a1918635e5e5bc8e452b168130541067cc339a7d72bd0abeb48174c691 Severity: high

Description: Description: There are multiple functions in the contract that lack deadline checks, potentially causing issues. For example, although the buyFor and sellFor functions implement _minAmountOut checks, the absence of a deadline check in these functions can result in a loss of funds for the user.

Scenario:

1. A user calculates that for 1000 _depositAmount, they will receive 900 shares at time A.
2. The user submits a buy order with 900 as _minAmountOut.
3. The transaction is submitted to the mempool. However, the user chose a transaction fee that is too low for miners to prioritize, causing the transaction to stay pending in the mempool for extended periods (hours, days, weeks, or longer).
4. When the average gas fee drops enough for the transaction to become interesting to miners, the price of each share has dropped. If recalculated, the user would see they could now get 950 shares.
5. However, the transaction was submitted with 900 as _minAmountOut, resulting in an execution that may not reflect the user's current expectations.
6. An even worse scenario involves MEV (Miner Extractable Value) exploitation, where malicious actors could deliberately execute the pending transaction at a time that benefits them.

Impact: The lack of deadline checks can lead to loss of funds for users due to outdated slippage parameters, especially in volatile markets or during extended transaction delays.

Mitigation: Introduce a deadline parameter to the buyFor and sellFor functions to ensure that transactions are executed within an acceptable time frame.

[FHieser]

This issue is essentially criticizing the mempool and fee structure that is inherent in the system. The minAmount out basically allows the user to specify with what amount the user is content with. This includes state where he has to wait for the transaction to run through. Also if he already waits a week he could just teminate his transaction and readd a new one

I would agree that a deadline parameter would be a nice to have, but thats it, a nice to have and not an issue

[0xmahdirostami]

@FHieser Thanks, but you are preventing frontrunning, slippage, and other MEV issues by half. The other half to prevent these issues is a deadline.

same issue in different competitions:

• Caviar https://solodit.xyz/issues/m-01-missing-deadline-checks-allow-pending-transactions-to-be-maliciously-executed-code4rena-caviar-caviar-contest-git
• backed-protocol https://solodit.xyz/issues/m-01-missing-deadline-checks-allow-pending-transactions-to-be-maliciously-executed-code4rena-backed-protocol-papr-contest-git
• bloom https://solodit.xyz/issues/m-01-the-swap-mechanism-does-not-have-a-deadline-parameter-pashov-none-bloom-markdown

and many many others I think this issue is valid.