Incorrect `AfEth.price()` calculation

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/6b4867491350f8327d0ac4f496f263642cf3c1be/contracts/AfEth.sol#L140

Vulnerability details

Impact

AfEth.price() may be calculated as too low.

Proof of Concept

AfEth.requestWithdraw() does not burn the afEth but only transfers it to itself. Hence the withdrawRatio is calculated using only the free supply of afEth: AfEth.sol#L180-L185

// ratio of afEth being withdrawn to totalSupply
// we are transfering the afEth to the contract when we requestWithdraw
// we shouldn't include that in the withdrawRatio
uint256 afEthBalance = balanceOf(address(this));
uint256 withdrawRatio = (_amount * 1e18) /
    (totalSupply() - afEthBalance);

Later the afEth is burned in withdraw() (AfEth.sol#L255).

Similar accounting is done for safEth: AfEth.sol#L195-L197

uint256 safEthBalance = safEthBalanceMinusPending();

uint256 safEthWithdrawAmount = (withdrawRatio * safEthBalance) / 1e18;

Whereas for vAfEth AfEth.sol#L191-L193

uint256 vEthWithdrawId = AbstractStrategy(vEthAddress).requestWithdraw(
    votiumWithdrawAmount
);

VotiumStrategy.requestWithdraw() just burns the vAfEth immediately (VotiumStrategy.sol#L60).

However, the AfEth.price() calculation does not use the same accounting for the effective supply of afEth: AfEth.sol#L133-L141

function price() public view returns (uint256) {
    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();
}

It uses totalSupply() instead of totalSupply() - balanceOf(address(this)). This means that with withdrawal requests outstanding the price() will be too low, and deposit() will thus mint too much afEth.

Recommended Mitigation Steps

Amending the price() calculation to use totalSupply() - balanceOf(address(this)) in the denominator would resolve this issue.

But a better solution is to just burn the afEth immediately in requestWithdraw(). Since requestWithdraw() immediately fixes the withdrawal in terms of safEth and CVX, it doesn't matter for the withdrawal when the afEth is burned.

Assessed type

ERC4626

[elmutt]

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

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as duplicate of #25

[c4-judge]

0xleastwood marked the issue as satisfactory