Lack of Deadline Protection in Key Functions Poses Potential Exploits

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L266-L490

Vulnerability details

Impact

The absence of a deadline parameter in key function calls within the EvolvingProteus contract poses a significant vulnerability. Transactions that stay pending in the mempool due to outdated slippage could be executed at a much later time than initially intended by the user. This delay can result in unfavorable trade outcomes, potentially causing financial losses for both users and the protocol. Malicious validators, especially in PoS systems where block proposers are pre-known, could exploit this vulnerability. They might deliberately delay the inclusion of certain transactions to take advantage of market conditions or even manipulate them to their advantage.

Proof of Concept

Although all key function (swapGivenInputAmount(), swapGivenOutputAmount(), depositGivenInputAmount(), depositGivenOutputAmount(), withdrawGivenOutputAmount(), and withdrawGivenInputAmount()) calls involving swaps, LP token minting/burning in EvolvingProteus.sol from LiquidityPool.sol have slippage protections, they fail to provide the important deadline parameter, as can been seen in the following example:

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L272-L277

    function swapGivenInputAmount(
        uint256 xBalance,
        uint256 yBalance,
        uint256 inputAmount,
        SpecifiedToken inputToken
    ) external view returns (uint256 outputAmount) {

This omission allows transactions with outdated slippage to stay in a pending state, vulnerable to delayed execution.

A related article titled "Why You Should Stop Using Block Timestamp as Deadline in Swaps" highlights that in PoS systems, block proposers are known in advance, ranging from a minimum of 6 minutes and 24 seconds to a maximum of 12 minutes and 48 seconds. This predictability can be abused by malicious actors to exploit the lack of a deadline.

Tools Used

Manual

Recommended Mitigation Steps

Add a deadline parameter to each function and then check if the current block timestamp exceeds this deadline. If it does, the transaction should revert. Here's how you can refactor the functions:

Add the deadline parameter to each function:

function swapGivenInputAmount(
    uint256 xBalance,
    uint256 yBalance,
    uint256 inputAmount,
    SpecifiedToken inputToken,
    uint256 deadline
) external view returns (uint256 outputAmount) {
    // ... rest of the code ...
}

// Add the deadline parameter to other functions similarly.

At the beginning of each function, check if the current block timestamp exceeds the deadline:

require(block.timestamp <= deadline, "Transaction deadline exceeded");

Here's how the refactored swapGivenInputAmount function would look:

function swapGivenInputAmount(
    uint256 xBalance,
    uint256 yBalance,
    uint256 inputAmount,
    SpecifiedToken inputToken,
    uint256 deadline
) external view returns (uint256 outputAmount) {
    require(block.timestamp <= deadline, "Transaction deadline exceeded");

    // ... rest of the code ...
}

Repeat this process for each of the other functions.

Note: While adding a deadline parameter to view functions can be done, it's a bit unconventional. View functions don't change state and are typically used to retrieve data. The deadline is more commonly added to functions that change state or involve transfers of value to ensure they don't get executed later than intended.

Assessed type

Timing

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #10

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid