Some functions in TokenisableRange.sol does not allow user to supply slippage and deadline.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L154
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L200
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L260
https://github.com/code-423n4/2023-08-goodentry/blob/71c0c0eca8af957202ccdbf5ce2f2a514ffe2e24/contracts/TokenisableRange.sol#L301

Vulnerability details

Impact

Not allowing users to supply their own deadline could potentially expose them to sandwich attacks

Proof of Concept

There are 04 instances of this issue:
TokenisableRange.sol#init() , TokenisableRange.sol#claimFee(), TokenisableRange.sol#deposit(), TokenisableRange.sol#withdraw()

File: TokenisableRange.sol
134:  function init(uint n0, uint n1) external {
---SKIP---
154:         deadline: block.timestamp
---------
167:  function claimFee() public {
---SKIP---
200:          deadline: block.timestamp
---------
222:  function deposit(uint256 n0, uint256 n1) external nonReentrant returns (uint256 lpAmt) {
---SKIP---
260:        deadline: block.timestamp
---------
292:  function withdraw(uint256 lp, uint256 amount0Min, uint256 amount1Min) external nonReentrant returns (uint256 removed0, uint256 removed1) {
---SKIP---
301:        deadline: block.timestamp

Passing block.timestamp as the expiry/deadline of an operation does not mean "require immediate execution" - it means "whatever block this transaction appears in, I'm comfortable with that block's timestamp". Providing this value means that a malicious miner can hold the transaction for as long as they like (think the flashbots mempool for bundling transactions), which may be until they are able to cause the transaction to incur the maximum amount of slippage allowed by the slippage parameter, or until conditions become unfavorable enough that other orders, e.g. liquidations, are triggered. Timestamps should be chosen off-chain, and should be specified by the caller to avoid unnecessary MEV.

Tools Used

Manual review

Recommended Mitigation Steps

Require the user to pass in a timestamp.

Assessed type

MEV

[c4-pre-sort]

141345 marked the issue as duplicate of #78

[c4-pre-sort]

141345 marked the issue as duplicate of #260

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Insufficient proof

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[Nadinnnn]

I would like to provide more of the same issues discussed before
code-423n4/2022-11-paraspace-findings#429
https://code4rena.com/reports/2022-06-canto-v2#m-01-stableswap---deadline-do-not-work

From a judge's contest in a previous contest

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”.
Due to the removal of the check, it may be more profitable for a miner to deny the transaction from being mined until the transaction incurs the maximum amount of slippage.

[gzeon-c4]

See #260 (comment)