AfEth price calculation doesn't factor locked tokens held in contract balance

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

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.

Impact

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 when VotiumStrategy::withdraw() is called). This means that calculation in line 140 will also be affected, vEthValueInEth and safEthValueInEth are calculated with the reduced position, but then those are divided by totalSupply(), which still counts for locked tokens.

Proof of Concept

1. User holds AfEth tokens and decides to withdraw by calling requestWithdraw().
2. Tokens are transferred from the user and locked into the contract. Total supply is not affected.
3. Corresponding token balances of SafEth and vAfEth are reduced in relation to the withdrawal. For SafEth, the pendingSafEthWithdraws is increased, making safEthBalanceMinusPending() account for this difference. For vAfEth, the balance held by the contract is reduced when the tokens are burned when VotiumStrategy::withdraw() is called.
4. At this point (and until the withdrawal is made effective when withdraw() is called), the implementation of price() will use the reduced balances to calculate safEthValueInEth and vEthValueInEth, but will still normalize them by totalSupply(), which considers the locked tokens in the contract.

Recommendation

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

Assessed type

Other

[elmutt]

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

[c4-judge]

0xleastwood marked the issue as duplicate of #59

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood marked the issue as duplicate of #25

[c4-judge]

0xleastwood marked the issue as satisfactory