Zero amount withdrawals of SafEth or Votium will brick the withdraw process

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

Withdrawals of amount zero from both SafEth and VotiumStrategy have issues downstream that will cause the transaction to revert, potentially bricking withdrawals from being executed.

Impact

Withdrawals in AfEth undergo a process to account for any potential delay when withdrawing locked tokens in the VotiumStrategy. When a withdrawal is requested, the implementation calculates the owed amounts for each token and queues the withdrawal. SafEth tokens will be reserved in the contract, and VotiumStrategy will also queue the withdrawal of CVX tokens.

When the time arrives, the user can call withdraw() to execute the withdrawal. This function will unstake from SafEth and withdraw from VotiumStrategy.

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

252:         ISafEth(SAF_ETH_ADDRESS).unstake(withdrawInfo.safEthWithdrawAmount, 0);
253:         AbstractStrategy(vEthAddress).withdraw(withdrawInfo.vEthWithdrawId);

Let's first consider the SafEth case. The current unstake() implementation in SafEth will revert if the unstaked amount is zero:

https://etherscan.io/address/0x591c4abf20f61a8b0ee06a5a2d2d2337241fe970#code#F1#L124

119:     function unstake(
120:         uint256 _safEthAmount,
121:         uint256 _minOut
122:     ) external nonReentrant {
123:         if (pauseUnstaking) revert UnstakingPausedError();
124:         if (_safEthAmount == 0) revert AmountTooLow();
125:         if (_safEthAmount > balanceOf(msg.sender)) revert InsufficientBalance();

As we can see in line 124, if _safEthAmount is zero the function will revert, and the transaction to withdraw() will revert too due to the bubbled error. This means that any withdrawal that ends up with a zero amount for SafEth will be bricked.

The VotiumStrategy case has a similar issue. The implementation of withdraw() will call sellCvx() to swap the owed amount of CVX for ETH. This is executed using a Curve pool, as we can see in the following snippet of code:

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

If we drill down in the Curve implementation, we can see that it validates that the input amount is greater than zero:

https://etherscan.io/address/0xB576491F1E6e5E62f1d8F26062Ee822B40B0E0d4#code#L714

709: def _exchange(sender: address, mvalue: uint256, i: uint256, j: uint256, dx: uint256, min_dy: uint256, use_eth: bool) -> uint256:
710:     assert not self.is_killed  # dev: the pool is killed
711:     assert i != j  # dev: coin index out of range
712:     assert i < N_COINS  # dev: coin index out of range
713:     assert j < N_COINS  # dev: coin index out of range
714:     assert dx > 0  # dev: do not exchange 0 coins

Again, this means that any withdrawal that ends up with a zero amount of vAfEth tokens (or the associated amount of CVX tokens) will be bricked when trying to execute the swap.

This can happen for different reasons. For example the current ratio may be 0 or 1e18, meaning the split goes entirely to SafEth or to VotiumStrategy. Another reason could be rounding, for small quantities the proportion may round down values to zero.

The critical issue is that both withdrawals are executed simultaneously. A zero amount shouldn't matter, but both happen at the time, and one may affect the other. If the SafEth amount is zero, it will brick the withdrawal for a potentially non-zero vAfEth amount. Similarly, if the vAfEth amount is zero, it will brick the withdrawal for a potentially non-zero SafEth amount

Proof of Concept

To simplify the case, let's say the current ratio is zero, meaning all goes to VotiumStrategy.

1. A user calls requestWithdraw(). Since currently the SafEth ratio is zero, the contract doesn't hold a position in SafEth. This means that safEthWithdrawAmount = 0, and the position is entirely in vAfEth (votiumWithdrawAmount > 0).
2. Time passes and the user can finally withdraw.
3. The user calls withdraw(). The implementation will try to call SafEth::unstake(0), which will cause an error, reverting the whole transaction.
4. The user will never be able to call withdraw(). Even if the ratios are changed, the calculated amount will be already stored in the withdrawIdInfo mapping. The withdrawal will be bricked, causing the loss of the vAfEth tokens.

Recommendation

For SafEth, avoid calling SafEth::unstake() if the calculated amount is zero:

+ if (withdrawInfo.safEthWithdrawAmount > 0) {
    ISafEth(SAF_ETH_ADDRESS).unstake(withdrawInfo.safEthWithdrawAmount, 0);
+ }

For VotiumStrategy, prevent requesting the withdrawal if votiumWithdrawAmount is zero, while also keeping track of this to also avoid executing the withdrawal when AfEth::withdraw() is called.

It is also recommended to add a guard in VotiumStrategy::withdraw() to avoid calling sellCvx() when cvxWithdrawAmount = 0.

-  uint256 ethReceived = sellCvx(cvxWithdrawAmount);
+  uint256 ethReceived = cvxWithdrawAmount > 0 ? sellCvx(cvxWithdrawAmount) : 0;

Assessed type

Other

[elmutt]

asymmetryfinance/afeth#159

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

It's unclear under what circumstances, withdrawRatio will be zero. As it appears, votiumWithdrawAmount is calculated as (withdrawRatio * votiumBalance) / 1e18 and similarly, safEthWithdrawAmount is calculated as (withdrawRatio * safEthBalance) / 1e18. So it seems the withdraw ratio is applied in the same manner to both of these amounts?

[0xleastwood]

The main case where this causes issues is when votiumBalance is non-zero and safEthBalance is zero or vice-versa. I'm curious as to when this might happen @elmutt ?

[c4-judge]

0xleastwood marked the issue as selected for report

[elmutt]

withdrawRatio

withdrawRatio represents the ratio of the amount being withdrawn to the total supply. So if a user owns 1% of afEth and withdraws their entire balance they will be set to receive 1% of each of the underlying assets (safEth & votiumStrategy) based on their current prices.

It should never be zero unless user is withdrawing the the last afEth from the system but we plan to solve this with an initial seed deposit

[c4-sponsor]

elmutt (sponsor) confirmed

[0xleastwood]

Okay good to know, I think I understand what you mean now. Issue appears valid and I think high severity is justified because the last staker would be unable to execute their withdrawal.

[0xleastwood]

However, can you explain why withdrawRatio would be zero upon the last withdrawal? It is calculated as (_amount * 1e18) / (totalSupply() - afEthBalance) where the denominator is equal to the _amount. Hence, this is equal to 1e18. So it attempts to withdraw all votium and safEth tokens from the contract.

A better thing to understand would be, when would either of this token balances be non-zero? And your mitigation is to seed the contract with some tokens initially so the token balance is always positive?

[elmutt]

I think we actually have a bug here. We shouldnt be subtracting afEthBalance.

Previously we subtracted it because the afEth contract held the users afEth before finally burning it on withdraw(). Now we just burn it on requestWithdraw() so we shouldn't be subtracting anymore.

does that make sense?

[0xleastwood]

Agreed, that makes sense. No need to track afEthBalance anymore. There might be other areas where this is being done incorrectly too.

[elmutt]

Agreed, that makes sense. No need to track afEthBalance anymore. There might be other areas where this is being done incorrectly too.

asymmetryfinance/afeth#172

fix for bug discussed