Missing deadline check for AfEth actions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L148 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L243

Vulnerability details

Summary

AfEth main actions execute on-chain swaps and lack an expiration deadline, which enables pending transactions to be maliciously executed at a later point.

Impact

Both AfEth deposits and withdrawals include on-chain swaps in AMM protocols as part of their execution, in order to convert the deposited ETH into the different underlying assets held by SafEth and the Votium strategy.

In the case of SafEth, depending on the derivative, staking may involve swapping ETH for other LSD. For example, the RocketPool derivative implementation uses Balancer to swap between ETH and rETH during deposits:

https://etherscan.io/address/0xb3e64c481f0fc82344a7045592284fddb9905b8b#code#F1#L157

function deposit() external payable onlyOwner returns (uint256) {
    uint256 rethBalanceBefore = IERC20(rethAddress()).balanceOf(
        address(this)
    );
    balancerSwap(msg.value);
    uint256 received = IERC20(rethAddress()).balanceOf(address(this)) -
        rethBalanceBefore;
    underlyingBalance = super.finalChecks(
        ethPerDerivative(true),
        msg.value,
        maxSlippage,
        received,
        true,
        underlyingBalance
    );
    return received;
}

In the case of Votium, deposited ETH is swapped to CVX in order to lock it in Convex. Similarly, when withdrawing, CVX tokens are swapped back to ETH. This is done using a Curve Pool in the buyCvx() and sellCvx() functions:

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L227-L265

227:     function buyCvx(
228:         uint256 _ethAmountIn
229:     ) internal returns (uint256 cvxAmountOut) {
230:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
231:         // eth -> cvx
232:         uint256 cvxBalanceBefore = IERC20(CVX_ADDRESS).balanceOf(address(this));
233:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying{
234:             value: _ethAmountIn
235:         }(
236:             0,
237:             1,
238:             _ethAmountIn,
239:             0 // this is handled at the afEth level
240:         );
241:         uint256 cvxBalanceAfter = IERC20(CVX_ADDRESS).balanceOf(address(this));
242:         cvxAmountOut = cvxBalanceAfter - cvxBalanceBefore;
243:     }
244: 
245:     /**
246:      * @notice - Internal utility function to sell cvx for eth
247:      * @param _cvxAmountIn - Amount of cvx to sell
248:      * @return ethAmountOut - Amount of eth received
249:      */
250:     function sellCvx(
251:         uint256 _cvxAmountIn
252:     ) internal returns (uint256 ethAmountOut) {
253:         address CVX_ETH_CRV_POOL_ADDRESS = 0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4;
254:         // cvx -> eth
255:         uint256 ethBalanceBefore = address(this).balance;
256:         IERC20(CVX_ADDRESS).approve(CVX_ETH_CRV_POOL_ADDRESS, _cvxAmountIn);
257: 
258:         ICrvEthPool(CVX_ETH_CRV_POOL_ADDRESS).exchange_underlying(
259:             1,
260:             0,
261:             _cvxAmountIn,
262:             0 // this is handled at the afEth level
263:         );
264:         ethAmountOut = address(this).balance - ethBalanceBefore;
265:     }

While both actions in AfEth, deposit() and withdraw(), have a minimum output parameter to control slippage, this doesn't offer protection against when the transaction is actually executed. If the price of the underlying assets drops while the transaction is pending, then the minimum output can still be fulfilled, but the user will get a bad rate due to the stale price. The outdated slippage value now allows for a high slippage trade in detriment of the user.

This can be also attacked by MEV bots which can still sandwich the transaction to profit on the difference. See this issue for an excellent reference on the topic (the author runs a MEV bot).

Proof of Concept

1. A user submits a transaction to deposit in AfEth.
2. The transaction sits in the mempool without being included in a block.
3. The price of CVX/ETH drops.
4. The transaction gets executed by the blockchain.
5. Since the price of CVX has dropped, the user will still get the minimum output expected but this will still represent less tokens than it would expect since the transaction has been delayed and the original price is now stale.

Recommendation

Add a deadline timestamp to the deposit() and withdraw() functions, and revert if this timestamp has passed.

Note also that the same should be applied to the VotiumStrategy contract if deposits and withdrawals are made directly there, without going through AfEth.

-   function deposit(uint256 _minout) external payable virtual {
+   function deposit(uint256 _minout, uint256 deadline) external payable virtual {
            if (pauseDeposit) revert Paused();
+           if (block.timestamp > deadline) revert StaleAction();

    function withdraw(
        uint256 _withdrawId,
-       uint256 _minout
+       uint256 _minout,
+       uint256 deadline
    ) external virtual onlyWithdrawIdOwner(_withdrawId) {
        if (pauseWithdraw) revert Paused();
+       if (block.timestamp > deadline) revert StaleAction();

Assessed type

MEV

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood removed the grade

[c4-sponsor]

elmutt (sponsor) confirmed

[elmutt]

https://github.com/asymmetryfinance/afeth/pull/175