Critical Bug: ReceiveFunds() DoS Risk In Treasury Contract

by Alex Johnson 59 views

This article discusses a critical vulnerability in the receiveFunds() function within the TreasuryBTC.sol smart contract, specifically on lines 79-86. This issue, discovered in the 360yuno-stack and nexalo ecosystems, can lead to a permanent denial of service (DoS) for a crucial accounting mechanism. The vulnerability arises from incomplete accounting practices that fail to track monthly reward deposits and reward claims accurately.

The Problem with receiveFunds()

The receiveFunds() function is designed to automatically track new deposits into the treasury by comparing the current stablecoin balance with an expected balance. This expected balance is calculated based on totalDeposited and totalWithdrawnForStaking. However, the logic used in this function makes a critical assumption: expectedBalance = totalDeposited - totalWithdrawnForStaking. This assumption is flawed because it doesn't account for two significant financial events that impact the contract's balance: monthly reward deposits and reward claims.

How Reward Deposits and Claims Break the Logic

  • Monthly Reward Deposits: The depositMonthlyRewards() function (lines 119-138) allows for the deposit of monthly rewards. While these deposits increase the contract's stablecoin balance, they do not update the totalDeposited variable. This means the balance grows, but the system's internal record of deposits remains unchanged, creating a discrepancy.
  • Reward Claims: When users claim their earned rewards using claimRewards or claimMultipleRewards (lines 158, 191), the contract's balance decreases. The system does track these claimed rewards in totalRewardsDistributed, but crucially, this variable is not incorporated into the receiveFunds() calculation. This omission further widens the gap between the actual balance and the expected balance calculated by receiveFunds().

A Step-by-Step Vulnerable Scenario

Let's walk through a scenario to illustrate how this vulnerability manifests:

  1. Initial Deposit: NexumManager deposits 10,000 USDC. At this point, balance is 10,000, totalDeposited is 10,000, and totalWithdrawnForStaking is 0. The receiveFunds() function would correctly calculate newDeposit as 0.
  2. Monthly Reward Deposit: The owner deposits 5,000 USDC as monthly BTC staking rewards using depositMonthlyRewards(5000e6). Now, the balance is 15,000. However, totalDeposited remains at 10,000 because these rewards aren't added to it. This is the core issue – the system isn't tracking these reward deposits.
  3. User Reward Claim: Users claim 2,000 USDC in rewards. The balance drops to 13,000. totalDeposited is still 10,000, but totalRewardsDistributed is now 2,000. This value, however, is ignored by receiveFunds().
  4. First receiveFunds() Call: Someone calls receiveFunds(). The function calculates the expectedBalance as 10,000 - 0 = 10,000. The actualBalance is 13,000. The newDeposit is calculated as 13,000 - 10,000 = 3,000. The function succeeds, but it incorrectly attributes these 3,000 as new deposits, adding them to totalDeposited. Now, totalDeposited becomes 13,000, effectively incorporating some of the reward pool into the main deposit accounting.
  5. Second User Reward Claim: Users claim another 4,000 USDC in rewards. The balance decreases to 9,000. totalDeposited is currently 13,000, and totalWithdrawnForStaking is 0.
  6. Second receiveFunds() Call (The Crash): Anyone calls receiveFunds() again. The function calculates expectedBalance as 13,000 - 0 = 13,000. The actualBalance is 9,000. The newDeposit calculation becomes 9,000 - 13,000. In Solidity versions 0.8+, this results in an arithmetic underflow, and the transaction reverts.
  7. Permanent DoS: Following this underflow, all subsequent calls to receiveFunds() will permanently revert. This is because the actual balance has fallen below the calculated expected balance, and this condition will persist due to the flawed accounting.

The Root Cause Explained

Essentially, monthly reward deposits temporarily inflate the contract's balance, masking the underlying accounting problem. When the total amount of claimed rewards eventually surpasses the amount of untracked monthly reward deposits, the actual balance dips below the incorrectly calculated expected balance. This triggers the underflow, rendering the receiveFunds() function unusable indefinitely.

Impact of the Vulnerability

The primary impact is a permanent denial of service for the receiveFunds() function. This function is crucial for the NexumManager's treasury fund reconciliation process. Its failure prevents the automatic accounting of funds entering the treasury contract.

While the depositFunds() function remains operational, allowing for direct deposits, the broken receiveFunds() function causes significant integration issues between NexumManager and TreasuryBTC. It hampers the ability to automatically reconcile balances and ensures that not all incoming funds are properly tracked by the system. This can lead to potential financial discrepancies and operational difficulties for the protocol.

Recommendations for a Robust Solution

To address this critical vulnerability, the accounting system needs a comprehensive redesign to ensure all balance-changing operations are meticulously tracked. This involves modifying the receiveFunds() function and introducing a new state variable to accurately reflect reward deposits.

Proposed Code Modifications

Here’s how the code can be improved:

  1. Add a New State Variable: Introduce uint256 public totalRewardDeposits; to keep a running tally of all reward amounts deposited into the contract.

  2. Modify depositMonthlyRewards(): Update the depositMonthlyRewards() function to increment the new totalRewardDeposits variable whenever rewards are deposited:

    // Add state variable
uint256 public totalRewardDeposits;

function depositMonthlyRewards(uint256 rewardAmount) external onlyOwner {
    require(rewardAmount > 0, "Amount must be > 0");
    
    require(
        stablecoin.transferFrom(msg.sender, address(this), rewardAmount),
        "Transfer failed"
    );
    
    totalRewardDeposits += rewardAmount; // Track reward deposits
    
    uint256 snapshotId = snapshotCount;
    uint256 totalSupply = nxlToken.totalSupply();
    
    rewardSnapshots[snapshotId] = RewardSnapshot({
        timestamp: block.timestamp,
        totalRewards: rewardAmount,
        totalNXLSupply: totalSupply,
        distributed: false
    });
    
    snapshotCount++;
    emit RewardsDeposited(snapshotId, rewardAmount);
}

  3. Revamp receiveFunds(): Modify the receiveFunds() function to incorporate the totalRewardDeposits and totalRewardsDistributed into its expected balance calculation. The new logic should be:

    function receiveFunds() external {
    uint256 balance = stablecoin.balanceOf(address(this));
    
    // Calculate expected balance including all tracked operations
    uint256 expectedBalance = totalDeposited 
        + totalRewardDeposits 
        - totalWithdrawnForStaking 
        - totalRewardsDistributed;
    
    if (balance > expectedBalance) {
        uint256 newDeposit = balance - expectedBalance;
        totalDeposited += newDeposit;
        emit FundsReceived(msg.sender, newDeposit);
    }
}

By implementing these changes, the expectedBalance calculation will accurately account for all funds entering and leaving the contract, including initial deposits, reward deposits, withdrawals for staking, and claimed rewards. This prevents the arithmetic underflow, resolves the DoS condition, and ensures the treasury contract maintains precise and reliable accounting under all operational circumstances.

For more information on smart contract security and best practices, you can refer to resources like ConsenSys Diligence or the OpenZeppelin Blog.