Lost yield due to using block.timestamp as deadline in swap(), increaseLiquidity() and decreaseLiquidity() (which also don't always have slippage checks)

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/PositionManager/OptionsPositionManager.sol#L480
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/PositionManager/OptionsPositionManager.sol#L502
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L154
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L200
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L260
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L301
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/PositionManager/OptionsPositionManager.sol#L135
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/PositionManager/OptionsPositionManager.sol#L424
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L331
https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/TokenisableRange.sol#L198-L199

Vulnerability details

Impact

MEV opportunities and yield lost due to the fact that the deadline argument in the swap(), increaseLiquidity() and decreaseLiquidity() is set to block.timestamp. Additionally, increaseLiquidity() and decreaseLiquidity() don't always specify minAmount0 and maxAmount0, leading to even more MEV.

Proof of Concept

By setting the deadline to block.timestamp, the transaction can be included at any block, enabling more MEV opportunities. The codebase offers some protection by setting minimum or maximum amounts for swaps, but increaseLiquidity() and decreaseLiquidity() sometimes have 0 amountMin arguments, exposing even more the protocol for MEV.

Thus, validators may wait out the transaction as long as they want to get the best fee, making the user lose funds and have to wait longer for the transaction execution. In the case of the increaseLiquidity() and decreaseLiquidity() calls with 0 minAmount, it represents even more yield loss.

Here is an example of a similar report.

Tools Used

Vscode, Foundry, Solodit

Recommended Mitigation Steps

Set a maximum deadline in the swap(), increaseLiquidity() and decreaseLiquidity() functions. Additionally, specify minimum amounts in all withdraw() calls of TokanisableRange, which currently are being called with 0 amount0min and 0 amount1min in the GeVault and OptionsPositionManager (see the links for the locations).

Assessed type

MEV

[c4-pre-sort]

141345 marked the issue as duplicate of #78

[141345]

timestamp as deadline is just like fill or kill order, which should be executed immediately or cancel.

[c4-pre-sort]

141345 marked the issue as duplicate of #260

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-c