Users will never be able to withdraw their claimed airdrop fully in ERC20Airdrop2.sol contract

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L117

Vulnerability details

Impact

Context:
The ERC20Airdrop2.sol contract is for managing Taiko token airdrop for eligible users, but the withdrawal is not immediate and is subject to a withdrawal window.

Users can claim their tokens within claimStart and claimEnd. Once the claim window is over at claimEnd, they can withdraw their tokens between claimEnd and claimEnd + withdrawalWindow. During this withdrawal period, the tokens unlock linearly i.e. the tokens only become fully withdrawable at claimEnd + withdrawalWindow.

Issue:
The issue is that once the tokens for a user are fully unlocked, the withdraw() function cannot be called anymore due to the ongoingWithdrawals modifier having a strict claimEnd + withdrawalWindow < block.timestamp check in its second condition.

Impact:
Although the tokens become fully unlocked when block.timestamp = claimEnd + withdrawalWindow, it is extremely difficult or close to impossible for normal users to time this to get their full allocated claim amount. This means that users are always bound to lose certain amount of their eligible claim amount. This lost amount can be small for users who claim closer to claimEnd + withdrawalWindow and higher for those who partially claimed initially or did not claim at all thinking that they would claim once their tokens are fully unlocked.

Proof of Concept

Coded POC

How to use this POC:

• Add the POC to test/team/airdrop/ERC20Airdrop2.t.sol
• Run the POC using forge test --match-test testAirdropIssue -vvv
• The POC demonstrates how alice was only able to claim half her tokens out of her total 100 tokens claimable amount.

      function testAirdropIssue() public {
        vm.warp(uint64(block.timestamp + 11));

        vm.prank(Alice, Alice);
        airdrop2.claim(Alice, 100, merkleProof);

        // Roll 5 days after
        vm.roll(block.number + 200);
        vm.warp(claimEnd + 5 days);

        airdrop2.withdraw(Alice);

        console.log("Alice balance:", token.balanceOf(Alice));

        // Roll 6 days after
        vm.roll(block.number + 200);
        vm.warp(claimEnd + 11 days);

        vm.expectRevert(ERC20Airdrop2.WITHDRAWALS_NOT_ONGOING.selector);
        airdrop2.withdraw(Alice);
    }

Logs

Logs:
  > MockERC20Airdrop @ 0x0000000000000000000000000000000000000000
    proxy      : 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a
    impl       : 0x2e234DAe75C793f67A35089C9d99245E1C58470b
    owner      : 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
    msg.sender : 0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38
    this       : 0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  Alice balance: 50

Tools Used

Manual Review

Recommended Mitigation Steps

In the modifier ongoingWithdrawals(), consider adding a buffer window in the second condition that gives users enough time to claim the fully unlocked tokens.

    uint256 constant bufferWindow = X mins/hours/days;

    modifier ongoingWithdrawals() {
        if (claimEnd > block.timestamp || claimEnd + withdrawalWindow < block.timestamp + bufferWindow) {
            revert WITHDRAWALS_NOT_ONGOING();
        }
        _;
    }

Assessed type

Timing

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[dantaik]

This seems to be a valid bug report.

[c4-sponsor]

dantaik (sponsor) acknowledged

[dantaik]

Fixed in taikoxyz/taiko-mono#16596

[c4-sponsor]

adaki2004 (sponsor) confirmed

[adaki2004]

I think the current status here is: confirmed. It is indeed a bug in the flow, while we removed Airdrop2, it is still a confirmed finding on the repo for auditing.

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report