When withdrawals are enqueued in AfEth, the implementation will remove the tokens from the caller and lock these in the contract until the withdrawal is made effective. These tokens still count in the supply, and must not be considered during price calculation.
Withdrawals in AfEth undergo a process to account for any potential delay when withdrawing locked tokens in the VotiumStrategy. During this window of time, AfEth tokens are transferred from the caller into the contract. This can be seen in the implementation of requestWithdraw()
:
https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L175-L187
175: function requestWithdraw(uint256 _amount) external virtual {
176: uint256 withdrawTimeBefore = withdrawTime(_amount);
177: if (pauseWithdraw) revert Paused();
178: latestWithdrawId++;
179:
180: // ratio of afEth being withdrawn to totalSupply
181: // we are transfering the afEth to the contract when we requestWithdraw
182: // we shouldn't include that in the withdrawRatio
183: uint256 afEthBalance = balanceOf(address(this));
184: uint256 withdrawRatio = (_amount * 1e18) /
185: (totalSupply() - afEthBalance);
186:
187: _transfer(msg.sender, address(this), _amount);
...
Line 187 transfers the tokens from the caller to the same AfEth contract, which are held until withdrawals are made effective, at which point the tokens are burned. Notice that these locked tokens are taken into account while calculating the withdrawRatio
in line 184-185.
However, the same consideration is not present in the calculation of price()
:
https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L133-L141
133: function price() public view returns (uint256) {
134: if (totalSupply() == 0) return 1e18;
135: AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
136: uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
137: safEthBalanceMinusPending()) / 1e18;
138: uint256 vEthValueInEth = (vEthStrategy.price() *
139: vEthStrategy.balanceOf(address(this))) / 1e18;
140: return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
141: }
As we can see in the previous snippet of code, the total supply of the token is referenced in lines 134 and 140 without subtracting the "locked" tokens held in the contract. This leads to different inconsistencies that will end up with an incorrect price value.
- If the single/last token holder requests a withdrawal, then those locked tokens are still counted in the supply. This means that the condition in line 134 will be false, where in reality it should be true since there is no circulating supply, and the return value of this function should be
1e18
. - When a holder requests a withdrawal, the implementation will reduce the balance of SafEth (by incrementing
pendingSafEthWithdraws
) and the balance of vAfEth (since those tokens are burned whenVotiumStrategy::withdraw()
is called). This means that calculation in line 140 will also be affected,vEthValueInEth
andsafEthValueInEth
are calculated with the reduced position, but then those are divided bytotalSupply()
, which still counts for locked tokens.
- User holds AfEth tokens and decides to withdraw by calling
requestWithdraw()
. - Tokens are transferred from the user and locked into the contract. Total supply is not affected.
- Corresponding token balances of SafEth and vAfEth are reduced in relation to the withdrawal. For SafEth, the
pendingSafEthWithdraws
is increased, makingsafEthBalanceMinusPending()
account for this difference. For vAfEth, the balance held by the contract is reduced when the tokens are burned whenVotiumStrategy::withdraw()
is called. - At this point (and until the withdrawal is made effective when
withdraw()
is called), the implementation ofprice()
will use the reduced balances to calculatesafEthValueInEth
andvEthValueInEth
, but will still normalize them bytotalSupply()
, which considers the locked tokens in the contract.
The issue could be fixed by directly burning the tokens when requestWithdraw()
is called, instead of transferring and locking them in the contract.
If these should still count in the totalSupply()
for external reference while the withdrawal is pending, then the implementation of price()
must factor them in the calculation:
function price() public view returns (uint256) {
+ uint256 _totalSupply = totalSupply() - balanceOf(address(this));
- if (totalSupply() == 0) return 1e18;
+ if (_totalSupply == 0) return 1e18;
AbstractStrategy vEthStrategy = AbstractStrategy(vEthAddress);
uint256 safEthValueInEth = (ISafEth(SAF_ETH_ADDRESS).approxPrice(true) *
safEthBalanceMinusPending()) / 1e18;
uint256 vEthValueInEth = (vEthStrategy.price() *
vEthStrategy.balanceOf(address(this))) / 1e18;
- return ((vEthValueInEth + safEthValueInEth) * 1e18) / totalSupply();
+ return ((vEthValueInEth + safEthValueInEth) * 1e18) / _totalSupply;
}