Missing slippage control when directly interacting with the VotiumStrategy contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39 https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L109

Vulnerability details

Summary

Direct deposits and withdrawals within VotiumStrategy lack any slippage controls, which opens up the possibility of sandwich attacks and Miner Extractable Value (MEV) exploits.

Impact

Interactions in the AfEth protocol often require the exchange of ETH for other assets. Users deposit ETH, and the protocol needs to convert it to other LSD tokens while staking in SafEth, and also to CVX tokens while depositing in the VotiumStrategy. Similarly, during withdrawals, the protocol converts those assets back to ETH.

While the AfEth contract implements slippage control in deposit() and withdraw() through a _minout parameter to ensure the return of a minimum number of assets after execution, the same is not true for the VotiumStrategy contract, which can be used in a standalone fashion.

The deposit() function in VotiumStrategy simple takes the received ETH amount and swaps it to CVX using buyCvx():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L39-L46

39:     function deposit() public payable override returns (uint256 mintAmount) {
40:         uint256 priceBefore = cvxPerVotium();
41:         uint256 cvxAmount = buyCvx(msg.value);
42:         IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
43:         ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
44:         mintAmount = ((cvxAmount * 1e18) / priceBefore);
45:         _mint(msg.sender, mintAmount);
46:     }

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

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:     }

In the previous snippet of code, line 239, we can see that the last argument to exchange_underlying() is zero, which is the minimum output amount. The comment reads "this is handled at the afEth level" which is partially true: it is only checked when being used from AfEth, but not for direct depositors of the VotiumStrategy.

The withdraw() function has the same issue when swapping CVX for ETH in sellCvx():

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L109-L29

109:     function withdraw(uint256 _withdrawId) external override {
110:         if (withdrawIdToWithdrawRequestInfo[_withdrawId].owner != msg.sender)
111:             revert NotOwner();
112:         if (!this.canWithdraw(_withdrawId)) revert WithdrawNotReady();
113: 
114:         if (withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn)
115:             revert AlreadyWithdrawn();
116: 
117:         relock();
118: 
119:         uint256 cvxWithdrawAmount = withdrawIdToWithdrawRequestInfo[_withdrawId]
120:             .cvxOwed;
121: 
122:         uint256 ethReceived = sellCvx(cvxWithdrawAmount);
123:         cvxUnlockObligations -= cvxWithdrawAmount;
124:         withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn = true;
125: 
126:         // solhint-disable-next-line
127:         (bool sent, ) = msg.sender.call{value: ethReceived}("");
128:         if (!sent) revert FailedToSend();
129:     }

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

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:     }

Again, in line 262 we see the same issue as before, the minimum output is configured to zero.

We can see that there is no slippage control at any level, the swap output amount is configured to zero, and there is no check of the number of minted tokens or the amount of ETH sent back to the user.

This implies that direct deposits or withdrawals in the VotiumStrategy contract are susceptible to arbitrary sandwich attacks by MEV bots, which could potentially result in a loss of funds for the user. In the case of a sandwiched deposit, there will be a reduced issuance of VotiumStrategy tokens, as the swap may yield fewer CVX tokens than anticipated. Similarly, a sandwiched withdrawal may lead to a reduced amount of ETH being returned to the user.

Proof of Concept

1. A user sends a transaction to deposit in VotiumStrategy.
2. A MEV bot sees the transaction, and swaps a large amount of ETH for CVX in the Curve Pool.
3. The user's transaction goes through and swap is executed in the manipulated pool.
4. The MEV bot swaps CVX back to ETH, and pockets the slippage difference.
5. The user gets less CVX tokens, which means less minted VotiumStrategy tokens.

Recommendation

Add a minimum output parameter to both deposit() and withdraw() in the VotiumStrategy contract.

The AfEth contract can still set these arguments as zero, since it already has slippage control, while allowing direct users of the VotiumStrategy contract to protect against MEV attacks.

-   function deposit() public payable override returns (uint256 mintAmount) {
+   function deposit(uint256 minOut) public payable override returns (uint256 mintAmount) {
        uint256 priceBefore = cvxPerVotium();
        uint256 cvxAmount = buyCvx(msg.value);
        IERC20(CVX_ADDRESS).approve(VLCVX_ADDRESS, cvxAmount);
        ILockedCvx(VLCVX_ADDRESS).lock(address(this), cvxAmount, 0);
        mintAmount = ((cvxAmount * 1e18) / priceBefore);
+       require(mintAmount >= minOut);
        _mint(msg.sender, mintAmount);
    }

-   function withdraw(uint256 _withdrawId) external override {
+   function withdraw(uint256 _withdrawId, uint256 minOut) external override {
        if (withdrawIdToWithdrawRequestInfo[_withdrawId].owner != msg.sender)
            revert NotOwner();
        if (!this.canWithdraw(_withdrawId)) revert WithdrawNotReady();

        if (withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn)
            revert AlreadyWithdrawn();

        relock();

        uint256 cvxWithdrawAmount = withdrawIdToWithdrawRequestInfo[_withdrawId]
            .cvxOwed;

        uint256 ethReceived = sellCvx(cvxWithdrawAmount);
        cvxUnlockObligations -= cvxWithdrawAmount;
        withdrawIdToWithdrawRequestInfo[_withdrawId].withdrawn = true;

+       require(ethReceived >= minOut);

        // solhint-disable-next-line
        (bool sent, ) = msg.sender.call{value: ethReceived}("");
        if (!sent) revert FailedToSend();
    }

Assessed type

MEV

[elmutt]

@toshiSat Im thinking another solution could be to lock down votiumStrategy so only afEth can interact with it

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood marked the issue as primary issue

[elmutt]

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

we went with locking it down so only afEth deposit or withdraw from votium strategy

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood marked the issue as duplicate of #23

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood marked the issue as partial-50

[0xleastwood]

Only giving 50% credit because it misses the depositRewards() edge case.

[0xleastwood]

Giving full credit as paired with #41, it covers all cases outlined in the primary issue.

[c4-judge]

0xleastwood marked the issue as full credit

[c4-judge]

0xleastwood marked the issue as satisfactory