Open c4-submissions opened 9 months ago
0xleastwood marked the issue as primary issue
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?
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 ?
0xleastwood marked the issue as selected for report
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
elmutt (sponsor) confirmed
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.
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?
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?
Agreed, that makes sense. No need to track afEthBalance
anymore. There might be other areas where this is being done incorrectly too.
Agreed, that makes sense. No need to track
afEthBalance
anymore. There might be other areas where this is being done incorrectly too.
https://github.com/asymmetryfinance/afeth/pull/172
fix for bug discussed
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
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
As we can see in line 124, if
_safEthAmount
is zero the function will revert, and the transaction towithdraw()
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 callsellCvx()
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: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
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 be0
or1e18
, 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.
requestWithdraw()
. Since currently the SafEth ratio is zero, the contract doesn't hold a position in SafEth. This means thatsafEthWithdrawAmount = 0
, and the position is entirely in vAfEth (votiumWithdrawAmount > 0
).withdraw()
. The implementation will try to callSafEth::unstake(0)
, which will cause an error, reverting the whole transaction.withdraw()
. Even if the ratios are changed, the calculated amount will be already stored in thewithdrawIdInfo
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:For VotiumStrategy, prevent requesting the withdrawal if
votiumWithdrawAmount
is zero, while also keeping track of this to also avoid executing the withdrawal whenAfEth::withdraw()
is called.It is also recommended to add a guard in
VotiumStrategy::withdraw()
to avoid callingsellCvx()
whencvxWithdrawAmount = 0
.Assessed type
Other