Skip to content

Reserve.md

Latest commit

 

History

History
386 lines (259 loc) · 13.5 KB

Reserve.md

File metadata and controls

386 lines (259 loc) · 13.5 KB

Reserve Protocol Audit Report

Issue 1: Hardcoded MAINNET_EID Violates LayerZero Best Practices

Severity: High

Summary

The hardcoded MAINNET_EID in BridgeL1.sol and BridgeL2.sol will cause a complete denial of service for cross-chain operations as LayerZero network upgrades will render the bridge contract permanently unusable.

Finding Description

In BridgeL1.sol and BridgeL2.sol, the MAINNET_EID is hardcoded as an immutable value 30101, violating LayerZero's integration best practices which explicitly warn against hardcoding endpoint IDs due to potential network upgrades and migrations.

Root Cause

The vulnerable code:

uint32 public immutable MAINNET_EID = 30101; // @audit Hardcoded EID

According to LayerZero documentation, hardcoding endpoint IDs violates best practices. The recommendation is to use admin-restricted setters to configure endpoint IDs instead of hardcoding them.

Internal Pre-conditions

  • Contract needs to be deployed with hardcoded MAINNET_EID = 30101
  • LayerZero infrastructure needs to undergo network upgrade changing mainnet endpoint ID
  • Bridge operations need to be actively used for cross-chain reserve synchronization

External Pre-conditions

  • LayerZero protocol needs to change mainnet endpoint ID from 30101 to a different value during network upgrade
  • LayerZero infrastructure needs to deprecate or disable the old endpoint ID 30101

Attack Path

  1. LayerZero announces network upgrade and changes mainnet endpoint ID from 30101 to 30102
  2. The old endpoint ID 30101 becomes deprecated or non-functional
  3. Protocol executor calls syncMainnetReserves() which uses hardcoded MAINNET_EID = 30101
  4. The function call to totalReservesOracle.setCrosschainReserves(MAINNET_EID, rzrReserves, usdReserves) uses invalid endpoint ID
  5. Cross-chain reserve synchronization fails permanently
  6. RebaseController cannot access accurate total reserves, breaking epoch calculations

Impact Explanation

The protocol suffers complete loss of cross-chain functionality. Bridge operations become permanently unusable, preventing:

Reserve Synchronization

L1 and L2 chains cannot synchronize reserve data, breaking the multi-chain architecture.

Broken Backing Ratio Calculations

Accurate backing ratio calculations for rebases become impossible without cross-chain reserve data.

Loss of Cross-Chain Functionality

Cross-chain liquid staking operations become permanently unavailable.

Protocol Breakdown

This effectively breaks the multi-chain protocol architecture with no recovery path without full contract redeployment.

Proof of Concept

The vulnerability manifests through the following scenario:

  1. LayerZero announces network upgrade and changes mainnet endpoint ID from 30101 to 30102
  2. The old endpoint ID 30101 becomes deprecated or non-functional
  3. Protocol executor calls syncMainnetReserves() which uses hardcoded MAINNET_EID = 30101
  4. The function call to totalReservesOracle.setCrosschainReserves(MAINNET_EID, rzrReserves, usdReserves) uses invalid endpoint ID
  5. Cross-chain reserve synchronization fails permanently
  6. RebaseController cannot access accurate total reserves, breaking epoch calculations

Recommendation

Replace the hardcoded immutable MAINNET_EID with a configurable state variable:

uint32 public mainnetEid; // Remove immutable keyword

constructor(
    uint32 _mainnetEid, // Add as constructor parameter
    uint32 _readChannel,
    address _endpoint,
    // ... other parameters
) OAppRead(_endpoint, msg.sender) Ownable(msg.sender) {
    mainnetEid = _mainnetEid; // Set during deployment
    // ... rest of constructor
}

// Add governance function to update EID if needed
function setMainnetEid(uint32 _newEid) external onlyGovernor {
    mainnetEid = _newEid;
    emit MainnetEidUpdated(_newEid);
}

Issue 2: Missing Access Control in increaseAmount() Enables Arbitrary Position Griefing

Severity: High

Summary

The missing ownership check in increaseAmount() can cause a complete loss of staker utility as an attacker can arbitrarily inflate the declaredValue of any position. This results in:

  • Streaming tax griefing — the attacker forces accelerated tax accrual that drains staked funds
  • Market lock attack — the attacker inflates declaredValue to unrealistic values, making the position permanently unbuyable

Finding Description

In AppStaking.sol, the increaseAmount() function lacks ownership or approval checks when updating declaredValue and taxPerSecond. Since declaredValue can only increase (monotonic), any griefing action is irreversible by the victim.

Root Cause

The vulnerable function:

function increaseAmount(uint256 tokenId, uint256 amount, uint256 declaredValue) external {
    // Missing: require(msg.sender == owner || msg.sender == approved);
    // ... rest of function
}

Internal Pre-conditions

  • A user has an active staking position (amount > 0)
  • The attacker knows the victim's tokenId
  • Victim has not yet withdrawn or exited the position

External Pre-conditions

None

Attack Path

Streaming Tax Grief

  1. Attacker calls increaseAmount(tokenId, 0, veryLargeDeclaredValue)
  2. Victim's taxPerSecond spikes
  3. Tax accrues automatically and is collected on the next interaction, draining the victim's staked funds

Market Lock Attack

  1. Attacker calls increaseAmount(tokenId, 0, astronomicallyLargeDeclaredValue)
  2. Declared value is now stuck at an unreasonably high level (cannot decrease)
  3. No buyer will ever purchase the position at this inflated price
  4. Victim's position is effectively locked — they cannot sell or recover utility from it

Impact Explanation

Stakers suffer either:

Fund Loss

Through forced streaming tax that drains staked funds automatically.

Permanent Illiquidity

Declared value locked at absurd levels, making exit impossible or resulting in complete loss of funds for the staker.

Zero-Cost Griefing

Attackers gain nothing financially but can grief victims at zero cost, causing severe reputational damage to the protocol.

Systemic Damage

Positions can be griefed into unusability, severely damaging the protocol's reputation and user trust.

Proof of Concept

function test_IncreaseAmountIssue() public {
    vm.startPrank(owner);

    // Create initial position
    app.mint(owner, STAKE_AMOUNT);
    app.approve(address(staking), STAKE_AMOUNT);
    (uint256 tokenId,) = staking.createPosition(owner, STAKE_AMOUNT, DECLARED_VALUE, 0);

    IAppStaking.Position memory initialPosition = staking.positions(tokenId);
    assertEq(initialPosition.amount, STAKE_AMOUNT);
    assertEq(initialPosition.declaredValue, DECLARED_VALUE);
    assertEq(staking.totalStaked(), STAKE_AMOUNT);

    vm.stopPrank();
    vm.warp(block.timestamp + 1 days);
    address attacker = makeAddr("attacker");

    // attacker inflate the declared value without having any funds
    vm.startPrank(attacker);
    staking.increaseAmount(tokenId, 0, type(uint160).max);
    vm.stopPrank();

    vm.warp(block.timestamp + 1 days);
    staking.claimRewards(tokenId);

    // mint DECLARED_VALUE + STAKE_AMOUNT to user to be able to buy
    vm.startPrank(owner);
    app.mint(user1, DECLARED_VALUE + STAKE_AMOUNT);
    vm.stopPrank();

    // user1 tries to buy asset, but value not reasonable
    vm.startPrank(user1);
    app.approve(address(staking), DECLARED_VALUE + STAKE_AMOUNT);
    vm.expectRevert();
    staking.buyPosition(tokenId);
    vm.stopPrank();
}

Test Output Analysis

The test demonstrates:

  • An attacker can call increaseAmount() on any position without ownership
  • The declared value is inflated to an unreasonable level (type(uint160).max)
  • The position becomes unbuyable due to the inflated price
  • The victim's position is effectively locked

Recommendation

Add an ownership/approval check in increaseAmount() to ensure only the owner or approved operator can raise declaredValue:

function increaseAmount(uint256 tokenId, uint256 amount, uint256 declaredValue) external {
    // Add ownership check
    require(
        msg.sender == ownerOf(tokenId) || msg.sender == getApproved(tokenId),
        "Not authorized"
    );
    
    // ... rest of function
}

Issue 3: Interest Can Be Claimed Twice via ClaimInterest + Redeem

Severity: High

Summary

The failure of redeem() to subtract previously claimed interest will cause a double interest payout for stakers as an attacker will first claim interest using claimInterest() and then redeem the same position to receive the full accumulated interest again.

Finding Description

In Convertibles.sol, the redeem() function transfers amountStaked + interestAccumulated without subtracting position.fixedInterestClaimed. This results in users being able to withdraw both previously claimed interest and the same interest again during redemption.

Root Cause

The vulnerable function:

function redeem(uint256 tokenId) external nonReentrant onlyOwnerOrAuthorized(tokenId) {
    // ... code ...
    uint256 totalInterest = _interestAccumulated(...);
    // Missing: uint256 alreadyClaimed = position.fixedInterestClaimed;
    // Missing: uint256 interestToPay = totalInterest > alreadyClaimed ? totalInterest - alreadyClaimed : 0;
    
    _burn(tokenId);
    delete _positions[tokenId];
    
    totalConvertible -= amountConvertible;
    asset.transfer(owner, amountStaked + totalInterest); // BUG: pays full interest again
}

Internal Pre-conditions

  • A staker must first call claimInterest(tokenId) to withdraw accrued interest
  • The position.fixedInterestClaimed variable is updated but not accounted for during redeem()
  • The same staker later calls redeem(tokenId) to withdraw principal + interest

External Pre-conditions

  • The protocol must hold enough liquidity of the underlying assetToken to pay both principal and interest

Attack Path

  1. Attacker stakes 100e18 tokens with a lock duration
  2. After lock expiry, attacker calls claimInterest(tokenId) and withdraws X interest
  3. Attacker then calls redeem(tokenId)
  4. redeem() pays back principal + X again, effectively double-paying the interest

Impact Explanation

Staker Gains

Stakers (honest users) and the protocol treasury suffer losses as attackers can drain extra interest. The attacker gains 100% of the interest twice.

Protocol Loss

Over multiple positions or large stakes, this can lead to a complete loss of interest reserves in the protocol.

Fund Drainage

The protocol's interest reserves are drained as attackers exploit the double-claim vulnerability.

Proof of Concept

function test_RedeemDoubleInterest() public {
    // --- Setup ---
    vm.startPrank(user1);
    uint256 amount = 100e18;
    uint256 lockDuration = 60 days;

    mockLoanToken.approve(address(convertibles), amount);

    (uint256 tokenId,,,,,) = convertibles.stake(mockLoanToken, amount, lockDuration, user1);
    vm.stopPrank();

    // --- Time travel to after lock ---
    vm.warp(block.timestamp + lockDuration + 1);

    // --- Ensure contract has enough liquidity for redemption ---
    vm.prank(owner);
    mockLoanToken.mint(address(convertibles), 50e18);

    // --- Balances before ---
    uint256 balanceBefore = mockLoanToken.balanceOf(user1);

    // --- Step 1: Claim interest once ---
    vm.startPrank(user1);
    (uint256 claimableBefore, ) = convertibles.claimableInterest(tokenId);
    convertibles.claimInterest(tokenId);
    uint256 balanceAfterClaim = mockLoanToken.balanceOf(user1);

    console.log("Interest claimed once:", balanceAfterClaim - balanceBefore);

    // --- Step 2: Redeem position (BUG: pays interest again) ---
    convertibles.redeem(tokenId);
    uint256 balanceAfterRedeem = mockLoanToken.balanceOf(user1);

    vm.stopPrank();

    // --- Expected vs Actual ---
    uint256 expected = balanceBefore + amount + claimableBefore;
    console.log("Expected final balance:", expected);
    console.log("Actual final balance:", balanceAfterRedeem);

    // balanceAfterRedeem have double interest because it has interest from claimInterest and the redeem function
    assertGt(balanceAfterRedeem, expected);
}

Test Output Analysis

The test demonstrates:

  • User claims interest once via claimInterest()
  • User then calls redeem() which pays the full accumulated interest again
  • The final balance is greater than expected (principal + interest claimed once)
  • The attacker receives double the interest

Recommendation

In Convertibles.sol, adjust the payout calculation in redeem() to account for previously claimed interest:

function redeem(uint256 tokenId) external nonReentrant onlyOwnerOrAuthorized(tokenId) {
    // .... code..
    uint256 totalInterest = _interestAccumulated(...);
    uint256 alreadyClaimed = position.fixedInterestClaimed;
    uint256 interestToPay = totalInterest > alreadyClaimed
        ? totalInterest - alreadyClaimed
        : 0;
    
    _burn(tokenId);
    delete _positions[tokenId];

    totalConvertible -= amountConvertible;
    asset.transfer(owner, amountStaked + interestToPay);

    //.... remain code
}

Explanation of the Fix

  • Original behavior: Transfers amountStaked + totalInterest without checking fixedInterestClaimed
  • Fixed behavior: Calculates interestToPay = totalInterest - alreadyClaimed to only pay unclaimed interest
  • Result: Users can only claim their interest once, either through claimInterest() or redeem(), but not both