Arithmetic Underflow in syncRewards() Causes Permanent Vault Lockup.md

Summary

The syncRewards() function in xERC4626.sol performs an unchecked subtraction at line 85 that will underflow and revert if the vault's token balance drops below storedTotalAssets + lastRewardAmount. This can occur due to slashing events, external bugs, or accounting errors. Once triggered, all vault operations become permanently blocked since the andSync modifier calls syncRewards() before every transaction.

Finding Description

The vulnerability exists in the reward calculation at xERC4626.sol:85:

function syncRewards() public virtual {
    // ...
    uint256 storedTotalAssets_ = storedTotalAssets;
    uint192 lastRewardAmount_ = lastRewardAmount;

    // VULNERABLE: Underflows if balance < storedTotalAssets_ + lastRewardAmount_
    uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

    // Rest of function never executes if above line reverts
    storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;
    lastRewardAmount = uint192(nextRewards);
    // ...
}

The Underflow Condition:

If: asset.balanceOf(address(this)) < storedTotalAssets + lastRewardAmount
Then: Subtraction underflows -> Revert with Panic(0x11)

How This Can Happen:

1. ETH Slashing Event (for sfrxETH):

   • Validators get slashed for misbehavior
   • frxETH balance decreases below expected amount
   • syncRewards() calculation underflows

2. External Integration Bug:

   • Another protocol interacts with the vault
   • Bug causes tokens to be transferred out
   • Balance drops below accounting expectations

3. Admin Error:

   • Admin accidentally uses recoverEther() or moveWithheldETH()
   • Removes more than the withheld amount
   • Creates accounting mismatch

4. Token Rebasing:

   • If underlying token is rebasing (e.g., stETH)
   • Negative rebase reduces balance
   • Accounting becomes inconsistent

Impact Explanation

Impact: MEDIUM-HIGH

1. Complete Vault Lockup: All deposit/withdraw/redeem operations blocked.

2. User Funds Trapped: Existing depositors cannot access their funds.

3. No Recovery Path: The contract has no mechanism to handle this scenario.

4. Cascading Failure: Any protocol integrating with sfrxETH is affected.

5. Time-Sensitive: For staking protocols, slashing is a known risk that should be handled gracefully.

Likelihood Explanation

Likelihood: MEDIUM

1. Slashing is Real: Ethereum validators do get slashed - it's part of the protocol design.

2. Historical Precedent: Multiple slashing events have occurred on mainnet.

3. Integration Risk: As DeFi composability increases, cross-protocol bugs become more likely.

4. Edge Case Not Handled: The code assumes balance always increases, which is not guaranteed.

Proof of Concept

Save as test/SyncRewardsUnderflow.t.sol and run:

forge test --match-contract SyncRewardsUnderflowPoC -vvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import "forge-std/console.sol";

// ============================================
// MOCK CONTRACTS
// ============================================

contract MockERC20 {
    string public name = "Mock Token";
    uint8 public decimals = 18;

    mapping(address => uint256) public balanceOf;
    mapping(address => mapping(address => uint256)) public allowance;
    uint256 public totalSupply;

    function mint(address to, uint256 amount) external {
        balanceOf[to] += amount;
        totalSupply += amount;
    }

    function burn(address from, uint256 amount) external {
        balanceOf[from] -= amount;
        totalSupply -= amount;
    }

    function transfer(address to, uint256 amount) external returns (bool) {
        balanceOf[msg.sender] -= amount;
        balanceOf[to] += amount;
        return true;
    }

    function transferFrom(address from, address to, uint256 amount) external returns (bool) {
        if (allowance[from][msg.sender] != type(uint256).max) {
            allowance[from][msg.sender] -= amount;
        }
        balanceOf[from] -= amount;
        balanceOf[to] += amount;
        return true;
    }

    function approve(address spender, uint256 amount) external returns (bool) {
        allowance[msg.sender][spender] = amount;
        return true;
    }
}

// Vulnerable xERC4626 vault
contract VulnerableXERC4626 {
    MockERC20 public immutable asset;

    mapping(address => uint256) public balanceOf;
    uint256 public totalSupply;

    uint256 public storedTotalAssets;
    uint192 public lastRewardAmount;
    uint32 public lastSync;
    uint32 public rewardsCycleEnd;
    uint32 public rewardsCycleLength;

    error SyncError();

    constructor(address _asset, uint32 _rewardsCycleLength) {
        asset = MockERC20(_asset);
        rewardsCycleLength = _rewardsCycleLength;
        lastSync = uint32(block.timestamp);
        rewardsCycleEnd = uint32(block.timestamp) + _rewardsCycleLength;
    }

    modifier andSync() {
        if (block.timestamp >= rewardsCycleEnd) {
            syncRewards();
        }
        _;
    }

    function totalAssets() public view returns (uint256) {
        if (block.timestamp >= rewardsCycleEnd) {
            return storedTotalAssets + lastRewardAmount;
        }
        uint256 unlockedRewards = (lastRewardAmount * (block.timestamp - lastSync)) / (rewardsCycleEnd - lastSync);
        return storedTotalAssets + unlockedRewards;
    }

    function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply;
        return supply == 0 ? assets : (assets * supply) / totalAssets();
    }

    function convertToAssets(uint256 shares) public view returns (uint256) {
        uint256 supply = totalSupply;
        return supply == 0 ? shares : (shares * totalAssets()) / supply;
    }

    function deposit(uint256 assets, address receiver) external andSync returns (uint256 shares) {
        shares = convertToShares(assets);
        require(shares != 0, "ZERO_SHARES");
        asset.transferFrom(msg.sender, address(this), assets);
        _mint(receiver, shares);
        storedTotalAssets += assets;
    }

    function withdraw(uint256 assets, address receiver, address owner) external andSync returns (uint256 shares) {
        shares = (assets * totalSupply) / totalAssets();
        if (shares == 0) shares = 1;
        _burn(owner, shares);
        storedTotalAssets -= assets;
        asset.transfer(receiver, assets);
    }

    function redeem(uint256 shares, address receiver, address owner) external andSync returns (uint256 assets) {
        assets = convertToAssets(shares);
        require(assets != 0, "ZERO_ASSETS");
        _burn(owner, shares);
        storedTotalAssets -= assets;
        asset.transfer(receiver, assets);
    }

    // VULNERABLE FUNCTION
    function syncRewards() public {
        if (block.timestamp < rewardsCycleEnd) revert SyncError();

        uint256 storedTotalAssets_ = storedTotalAssets;
        uint192 lastRewardAmount_ = lastRewardAmount;

        // VULNERABILITY: This line underflows if balance < storedTotalAssets_ + lastRewardAmount_
        uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;
        lastRewardAmount = uint192(nextRewards);

        uint32 timestamp = uint32(block.timestamp);
        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
        if (end - timestamp < rewardsCycleLength / 20) {
            end += rewardsCycleLength;
        }
        rewardsCycleEnd = end;
        lastSync = timestamp;
    }

    function _mint(address to, uint256 amount) internal {
        balanceOf[to] += amount;
        totalSupply += amount;
    }

    function _burn(address from, uint256 amount) internal {
        balanceOf[from] -= amount;
        totalSupply -= amount;
    }
}

// ============================================
// PROOF OF CONCEPT TEST
// ============================================

contract SyncRewardsUnderflowPoC is Test {
    MockERC20 public token;
    VulnerableXERC4626 public vault;

    address public user1 = makeAddr("user1");
    address public user2 = makeAddr("user2");

    uint32 constant REWARDS_CYCLE = 7 days;

    function setUp() public {
        vm.warp(1704067200); // Jan 1, 2024

        token = new MockERC20();
        vault = new VulnerableXERC4626(address(token), REWARDS_CYCLE);

        // Fund users
        token.mint(user1, 1000 ether);
        token.mint(user2, 1000 ether);
    }

    function test_Underflow_SlashingScenario() public {
        console.log("========================================");
        console.log("  SYNCREWARDS UNDERFLOW - SLASHING");
        console.log("========================================");

        // STEP 1: Users deposit funds
        vm.startPrank(user1);
        token.approve(address(vault), type(uint256).max);
        vault.deposit(100 ether, user1);
        vm.stopPrank();

        console.log("\n[STEP 1] User1 deposits 100 ETH");
        console.log("  storedTotalAssets:", vault.storedTotalAssets() / 1e18, "ETH");
        console.log("  Vault balance:", token.balanceOf(address(vault)) / 1e18, "ETH");

        // STEP 2: Add rewards
        token.mint(address(vault), 10 ether);
        console.log("\n[STEP 2] 10 ETH rewards added");
        console.log("  Vault balance:", token.balanceOf(address(vault)) / 1e18, "ETH");

        // STEP 3: First sync (works fine)
        vm.warp(block.timestamp + REWARDS_CYCLE + 1);
        vault.syncRewards();

        console.log("\n[STEP 3] First syncRewards() - SUCCESS");
        console.log("  storedTotalAssets:", vault.storedTotalAssets() / 1e18, "ETH");
        console.log("  lastRewardAmount:", uint256(vault.lastRewardAmount()) / 1e18, "ETH");
        console.log("  Vault balance:", token.balanceOf(address(vault)) / 1e18, "ETH");

        // STEP 4: SLASHING EVENT - Remove tokens from vault
        console.log("\n[STEP 4] SLASHING EVENT OCCURS!");
        uint256 slashAmount = 20 ether;
        token.burn(address(vault), slashAmount);
        console.log("  Slashed:", slashAmount / 1e18, "ETH");
        console.log("  Vault balance:", token.balanceOf(address(vault)) / 1e18, "ETH");
        console.log("  storedTotalAssets:", vault.storedTotalAssets() / 1e18, "ETH");
        console.log("  lastRewardAmount:", uint256(vault.lastRewardAmount()) / 1e18, "ETH");

        // Calculate the underflow
        uint256 balance = token.balanceOf(address(vault));
        uint256 stored = vault.storedTotalAssets();
        uint256 lastReward = uint256(vault.lastRewardAmount());

        console.log("\n[UNDERFLOW ANALYSIS]");
        console.log("  balance:", balance / 1e18, "ETH");
        console.log("  stored + lastReward:", (stored + lastReward) / 1e18, "ETH");
        console.log("  balance < stored + lastReward:", balance < stored + lastReward);

        if (balance < stored + lastReward) {
            console.log("  >>> UNDERFLOW WILL OCCUR!");
        }

        // STEP 5: Advance time and try to interact
        vm.warp(block.timestamp + REWARDS_CYCLE + 1);

        console.log("\n[STEP 5] Attempting vault operations...");

        // Try syncRewards directly - should revert
        console.log("  Calling syncRewards()...");
        vm.expectRevert();  // Arithmetic underflow
        vault.syncRewards();
        console.log("  >>> syncRewards() REVERTED!");

        // Try deposit - should revert (andSync modifier)
        vm.startPrank(user2);
        token.approve(address(vault), type(uint256).max);
        console.log("  Calling deposit()...");
        vm.expectRevert();
        vault.deposit(10 ether, user2);
        console.log("  >>> deposit() REVERTED!");
        vm.stopPrank();

        // Try withdraw - should revert (andSync modifier)
        vm.startPrank(user1);
        console.log("  Calling redeem()...");
        vm.expectRevert();
        vault.redeem(vault.balanceOf(user1), user1, user1);
        console.log("  >>> redeem() REVERTED!");
        vm.stopPrank();

        console.log("\n========================================");
        console.log("  VAULT COMPLETELY LOCKED!");
        console.log("========================================");
        console.log("  User1 has", vault.balanceOf(user1) / 1e18, "shares");
        console.log("  User1 CANNOT withdraw funds!");
        console.log("  All operations permanently blocked!");
        console.log("========================================");
    }

    function test_Underflow_AccountingError() public {
        console.log("========================================");
        console.log("  SYNCREWARDS UNDERFLOW - ACCOUNTING");
        console.log("========================================");

        // Setup with deposits
        vm.startPrank(user1);
        token.approve(address(vault), type(uint256).max);
        vault.deposit(100 ether, user1);
        vm.stopPrank();

        // Add rewards and sync
        token.mint(address(vault), 50 ether);
        vm.warp(block.timestamp + REWARDS_CYCLE + 1);
        vault.syncRewards();

        console.log("\n[INITIAL STATE]");
        console.log("  Vault balance:", token.balanceOf(address(vault)) / 1e18, "ETH");
        console.log("  storedTotalAssets:", vault.storedTotalAssets() / 1e18, "ETH");
        console.log("  lastRewardAmount:", uint256(vault.lastRewardAmount()) / 1e18, "ETH");

        // Simulate external bug removing tokens
        console.log("\n[EXTERNAL BUG] Tokens removed from vault");
        token.burn(address(vault), 80 ether);

        console.log("  New balance:", token.balanceOf(address(vault)) / 1e18, "ETH");

        // Next cycle - vault is bricked
        vm.warp(block.timestamp + REWARDS_CYCLE + 1);

        console.log("\n[ATTEMPTING SYNC]");
        vm.expectRevert();
        vault.syncRewards();
        console.log("  >>> UNDERFLOW - Vault bricked!");

        console.log("\n========================================");
        console.log("  VULNERABILITY CONFIRMED");
        console.log("========================================");
    }

    function test_SafeSyncRewards_Mitigation() public {
        console.log("========================================");
        console.log("  MITIGATION: SAFE SYNC REWARDS");
        console.log("========================================");

        console.log("\n[CURRENT VULNERABLE CODE]");
        console.log("  uint256 nextRewards = balance - stored - last;");
        console.log("  // Reverts if balance < stored + last");

        console.log("\n[RECOMMENDED FIX]");
        console.log("  if (balance < stored + last) {");
        console.log("      // Handle slashing gracefully");
        console.log("      uint256 loss = (stored + last) - balance;");
        console.log("      storedTotalAssets -= loss; // Distribute loss");
        console.log("      lastRewardAmount = 0;");
        console.log("  } else {");
        console.log("      uint256 nextRewards = balance - stored - last;");
        console.log("      lastRewardAmount = uint192(nextRewards);");
        console.log("  }");

        console.log("\n[BENEFITS]");
        console.log("  1. Vault continues operating after slashing");
        console.log("  2. Users can withdraw (at reduced rate)");
        console.log("  3. No permanent lockup");
        console.log("  4. Graceful degradation");

        console.log("\n========================================");
    }
}

Expected Output:

[PASS] test_Underflow_SlashingScenario()
[PASS] test_Underflow_AccountingError()
[PASS] test_SafeSyncRewards_Mitigation()

[STEP 4] SLASHING EVENT OCCURS!
  Slashed: 20 ETH
  Vault balance: 90 ETH
  storedTotalAssets: 100 ETH
  lastRewardAmount: 10 ETH

[UNDERFLOW ANALYSIS]
  balance: 90 ETH
  stored + lastReward: 110 ETH
  balance < stored + lastReward: true
  >>> UNDERFLOW WILL OCCUR!

[STEP 5] Attempting vault operations...
  >>> syncRewards() REVERTED!
  >>> deposit() REVERTED!
  >>> redeem() REVERTED!

========================================
  VAULT COMPLETELY LOCKED!
========================================

Recommendation

Primary Fix - Handle balance reduction gracefully:

function syncRewards() public virtual {
    if (block.timestamp < rewardsCycleEnd_) revert SyncError();

    uint256 storedTotalAssets_ = storedTotalAssets;
    uint192 lastRewardAmount_ = lastRewardAmount;
    uint256 balance = asset.balanceOf(address(this));
    uint256 expected = storedTotalAssets_ + lastRewardAmount_;

    if (balance < expected) {
        // SLASHING/LOSS SCENARIO: Handle gracefully
        uint256 loss = expected - balance;

        // Distribute loss proportionally
        if (loss <= lastRewardAmount_) {
            lastRewardAmount = uint192(uint256(lastRewardAmount_) - loss);
        } else {
            storedTotalAssets = balance;
            lastRewardAmount = 0;
        }

        emit SlashingDetected(loss);
    } else {
        // Normal case: Calculate new rewards
        uint256 nextRewards = balance - expected;
        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;
        lastRewardAmount = uint192(nextRewards);
    }

    // Update timing
    lastSync = uint32(block.timestamp);
    rewardsCycleEnd = calculateNextCycleEnd();
}

Alternative Fix - Use checked subtraction with fallback:

function syncRewards() public virtual {
    // ...
    uint256 balance = asset.balanceOf(address(this));
    uint256 expected = storedTotalAssets_ + lastRewardAmount_;

    // Safe subtraction with zero fallback
    uint256 nextRewards = balance > expected ? balance - expected : 0;

    // Continue with normal flow
    storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;
    lastRewardAmount = uint192(nextRewards);
}

References

• SWC-101: Integer Overflow and Underflow
• Ethereum Validator Slashing Conditions
• OpenZeppelin Math Library