Some users may not be able to withdraw until rewardsCycleEnd due to underflow in beforeWithdraw() function

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x2b1e989f7e706213581585c76a599ab7c9b5daa5dac1eefd1d23626837dbe6d4 Severity: medium

Description:

Title

Some users may not be able to withdraw until rewardsCycleEnd due to underflow in beforeWithdraw() function

Severity

Medium

Affected contracts

wrstMTRG.sol, wstARB.sol, wstDOJ.sol, wstMANTA.sol, wstMETIS.sol, wstROSE.sol, wstVLX.sol, wstZETA.sol and wstToken.sol

Vulnerability details

beforeWithdraw() is called in withdraw() and redeem() function which updates the state of storedTotalAssets variable.

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

storedTotalAssets is a cached value of total assets which will only include the unlockedRewards when the whole rewards cycle ends.

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

With current implementation of beforeWithdraw(), it possible for storedTotalAssets -= amount; to revert when the withdrawal amount exceeds storedTotalAssets, as the withdrawal amount may include part of the unlockedRewards in the current cycle.

Consider below scenario:

1) Consider the rewardsCycleLength is 100 days. 2) Alice deposit() 100 stROSE tokens; 3) The owner transferred 100 stROSE tokens as rewards and called syncRewards(); 4) 1 day later, Alice redeem() with all shares, the transaction will revert at xERC4626.beforeWithdraw(). 5) Alice's shares worth 101 stROSE at this moment, but storedTotalAssets = 100, making storedTotalAssets -= amount reverts due to underflow.

6) Bob deposit() 1 stROSE tokens; 7) Alice withdraw() 101 stROSE tokens, storedTotalAssets becomes 0; 8) Bob can't even withdraw 1 wei of stROSE token, as storedTotalAssets is now 0. 9) If there are no new deposits, both Alice and Bob won't be able to withdraw any of their funds until rewardsCycleEnd.

Recommendation to fix

Consider below changes:

    function beforeWithdraw(uint256 amount, uint256 shares) internal virtual override {
        super.beforeWithdraw(amount, shares);
-        storedTotalAssets -= amount;
+    uint256 _storedTotalAssets = storedTotalAssets;
+    if (amount >= _storedTotalAssets) {
+        uint256 _totalAssets = totalAssets();
+        // _totalAssets - _storedTotalAssets == unlockedRewards
+        lastRewardAmount -= _totalAssets - _storedTotalAssets;
+        lastSync = block.timestamp;
+        storedTotalAssets = _totalAssets - amount;
+    } else {
+        storedTotalAssets = _storedTotalAssets - amount;
+    }
  }

Additional notes

This test by xERC4626 proves the issue exists. This underflow issue is officially made known by xERC4626 which can be checked here

Similar issues found in other forks:

https://github.com/code-423n4/2022-12-gogopool-findings/issues/317

https://github.com/code-423n4/2022-04-xtribe-findings/issues/48