Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L65-L68

Vulnerability details

Impact

Valid withdrawals will fail in certain edge cases

Proof of Concept

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L45-L62

function totalAssets() public view override returns (uint256) {
    // cache global vars
    uint256 storedTotalAssets_ = storedTotalAssets;
    uint192 lastRewardAmount_ = lastRewardAmount;
    uint32 rewardsCycleEnd_ = rewardsCycleEnd;
    uint32 lastSync_ = lastSync;

    if (block.timestamp >= rewardsCycleEnd_) {
        // no rewards or rewards fully unlocked
        // entire reward amount is available
        return storedTotalAssets_ + lastRewardAmount_;
    }

    // rewards not fully unlocked
    // add unlocked rewards to stored total
    uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
    return storedTotalAssets_ + unlockedRewards;
}

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L65-L68

function beforeWithdraw(uint256 amount, uint256 shares) internal virtual override {
    super.beforeWithdraw(amount, shares);
    storedTotalAssets -= amount;
}

When using redeem from ERC4626, previewRedeem is used to determine the number of assets to send the user for burning their shares. When calculating totalAssets, it factors in both storedTotalAssets and lastRewardAmount. The issue is that in xERC4626.sol#beforeWithdraw, it subtracts this amount from storedTotalAssets. Since this amount also includes lastRewardAmount it is possible that amount > storedTotalAssets which will cause and underflow and revert.

Example:

supply = 100 storedTotalAssets = 100 lastRewardAmount = 1

block.timestamp = rewardCycleEnd

Imagine a user try to withdraw 100 shares. totalAssets will return 101. Since all shares are being withdrawn, 101 will be passed as assets into beforeWithdraw. This will cause an underflow because assets > storedTotalAssets.

Tools Used

Recommended Mitigation Steps

This is an extreme edge case. Authors should evaluate if it is worth mitigating or accepting risk

code-423n4 / 2022-09-frax-findings