LACK OF DEADLINE PROTECTION IN THE `Curve2PoolAdapter.primitiveOutputAmount` AND `CurveTricryptoAdapter.primitiveOutputAmount` COULD RESULT IN BAD TRADES USING CURVE POOLS

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L162-L175 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L198-L227

Vulnerability details

Impact

The Curve2PoolAdapter.primitiveOutputAmount and CurveTricryptoAdapter.primitiveOutputAmount functions are used to execute swaps/add liquidity/remove liquidity on the Curve2 pool and Curve Tricrypto Pool respectively.

In both of the above primitiveOutputAmount functions the curve pools are accessed via the exchange, add_liquidity and the remove_liquidity_one_coin functions. The inherent slippage protections of these functions are not used since 0 is passed for the relevant slippage variable during execution of the above mentioned curve pool functions.

Slippage protection is performed separately inside the primitiveOutputAmount function after the decimal conversion of the output tokens as shown below:

    if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED(); 

The issue is even though the slippage protection is implemented for the transactions with the curve pools the deadline check is not implemented in the primitiveOutputAmount functions. As a result the curve pool transactions such as swaps, add liquidity and remove liquidity could be executed at a later point in time which could prompt the user to perform bad trades.

Due to the lack of a deadline check the following two scenarios can occur:

1. For example if the gasfee provided to the exchange transaction is low for the miners due to high average gas fee present in the mempool due to congestion. Hence the exchange transaction will only execute once the average gas fees drop so the miners will be interested in executing this transaction. But by then the underlying value of the output token would have dropped due to changing market dynamics (For example if the output token is weth then the underlying value of weth denominated in USD might have dropped by 200USD by the time exchange transaction was executed after the delay).

2. The MEV bots can perform sandwich attacks on the exchange trade. If the weth price has dropped compared to the input token significantly thus allowing the trader to gain more weth tokens after the exchange. But since the exchange transaction is performed after a delay the slippage protection gets outdated which would allow significant slippage. As a result a MEV bot could exploit this sandwiching the exchange transaction resulting in significant profit and large loss to the trader due to outdated slippage protection.

Proof of Concept

        if (action == ComputeType.Swap) {
            rawOutputAmount =
                ICurve2Pool(primitive).exchange(indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0);
        } else if (action == ComputeType.Deposit) {
            uint256[2] memory inputAmounts;
            inputAmounts[uint256(int256(indexOfInputAmount))] = rawInputAmount;
            rawOutputAmount = ICurve2Pool(primitive).add_liquidity(inputAmounts, 0);
        } else {
            rawOutputAmount = ICurve2Pool(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
        }

        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

        if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED();

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L162-L175

        if (action == ComputeType.Swap) {
            bool useEth = inputToken == zToken || outputToken == zToken;

            ICurveTricrypto(primitive).exchange{ value: inputToken == zToken ? rawInputAmount : 0 }(
                indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0, useEth
            );
        } else if (action == ComputeType.Deposit) {
            uint256[3] memory inputAmounts;
            inputAmounts[indexOfInputAmount] = rawInputAmount;

            if (inputToken == zToken) IWETH(underlying[zToken]).deposit{ value: rawInputAmount }();

            ICurveTricrypto(primitive).add_liquidity(inputAmounts, 0);
        } else {
            if (outputToken == zToken) {
                uint256 wethBalance = IERC20Metadata(underlying[zToken]).balanceOf(address(this));
                ICurveTricrypto(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
                IWETH(underlying[zToken]).withdraw(
                    IERC20Metadata(underlying[zToken]).balanceOf(address(this)) - wethBalance
                );
            } else {
                ICurveTricrypto(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
            }
        }

        uint256 rawOutputAmount = _getBalance(underlying[outputToken]) - _balanceBefore;

        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

        if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED();

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L198-L227

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence while interacting with DEXes like curve it is recommended to implement both the slippage and deadline protection. Since the slippage protection is already implemented separately without using the inherent slippage protection of curve functions, it is recommended to implement the deadline protection separately in the primitiveOutputAmount function (since curve liquidity pools do not inherently have deadline protection implemented). This can be done by passing in a input timestamp by the user indicating the latest time the curve transaction can be executed and if the current block.timestamp has exceeded this timestamp then the transaction should revert.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #214

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[viraj124]

disagreeing with severity since curve by default doesn't implement this but this can be a qa issue and we'll think around having a check for deadline

[c4-sponsor]

viraj124 (sponsor) acknowledged

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

I agree with the sponsor. I think the protocol acts here as a proxy to interact with other protocols. Shell doesn't have to provide any more security than interacting directly with the protocol. Marking as low

[0xA5DF]

Warden has 3 lows - this, #271 and #273

[c4-judge]

0xA5DF marked the issue as grade-b

[0xA5DF]

+L from #294