Users may lose airdropped tokens in ERC20Airdrop2 contract

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L39-L44
https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L88-L94
https://github.com/code-423n4/2024-03-taiko/blob/main/packages/protocol/contracts/team/airdrop/ERC20Airdrop2.sol#L114-L120

Vulnerability details

Issue Description

During an airdrop organized with the ERC20Airdrop2 contract, there are two steps involved: first, the chosen users must claim their airdrop amounts by calling the claim function, and then they must wait until the withdrawal period begins to start withdrawing those amounts using the withdraw function.

The withdrawal period operates similarly to a vesting period, during which initially block.timestamp < claimEnd, no amount can be withdrawn. Subsequently, users can gradually withdraw as time passes until the moment block.timestamp == claimEnd + withdrawalWindow, when the entire amount becomes withdrawable.

In typical vesting scenarios, the full airdropped amount remains withdrawable after the withdrawal period has ended. However, in the case of ERC20Airdrop2, this is not the case due to the presence of the ongoingWithdrawals modifier within the withdraw function:

function withdraw(address user) external ongoingWithdrawals {
    (, uint256 amount) = getBalance(user);
    withdrawnAmount[user] += amount;
    IERC20(token).transferFrom(vault, user, amount);

    emit Withdrawn(user, amount);
}

The ongoingWithdrawals modifier allows users to withdraw only during the withdrawal period, defined as claimEnd <= block.timestamp <= claimEnd + withdrawalWindow, as illustrated in the code below:

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

It's important to note that the full airdropped amount becomes withdrawable only when block.timestamp == claimEnd + withdrawalWindow, as enforced by the getBalance function invoked within the withdraw function:

function getBalance(address user)
    public
    view
    returns (uint256 balance, uint256 withdrawableAmount)
{
    balance = claimedAmount[user];
    // If balance is 0 then there is no balance and withdrawable amount
    if (balance == 0) return (0, 0);
    // Balance might be positive before end of claiming (claimEnd - if claimed already) but
    // withdrawable is 0.
    if (block.timestamp < claimEnd) return (balance, 0);

    // Hard cap timestamp - so range cannot go over - to get more allocation over time.
    uint256 timeBasedAllowance = balance
        * (block.timestamp.min(claimEnd + withdrawalWindow) - claimEnd) / withdrawalWindow;

    withdrawableAmount = timeBasedAllowance - withdrawnAmount[user];
}

This setup can lead to users losing a part or all of their airdropped tokens.

Let's consider two scenarios to illustrate this issue:

- Scenario 1 (relatively low impact):

• Bob was airdropped 1000 tokens which he claimed using the claim function.

• After 80% of the withdrawal period (withdrawalWindow) has passed, Bob decides to withdraw some of his airdropped tokens.

• According to getBalance, he should receive 80% of his total airdropped amount (the maximum withdrawable amount at that time), so Bob receives 800 tokens.

• When the withdrawal period (withdrawalWindow) is about to end, Bob submits a transaction to withdraw the remaining balance of his airdrop, 1 hour before the end (claimEnd + withdrawalWindow).

• However, Bob's transaction remains pending for more than 1 hour, and when it finally gets executed, the withdrawal period has ended, causing Bob's withdraw call to revert.

• As ongoingWithdrawals only allows withdrawals during the specified period, Bob will not be able to call withdraw again, resulting in the loss of his remaining 200 airdropped tokens.

- Scenario 2 (higher impact):

• Alice was airdropped 1000 tokens which she claimed using the claim function.

• Alice chooses not to withdraw until the last moment (the end of the withdrawal period) to receive her full airdropped amount.

• As the withdrawal period (withdrawalWindow) is about to end, Alice submits a transaction to withdraw the remaining balance of her airdrop, 1 hour before the period ends (claimEnd + withdrawalWindow).

• However, Alice's transaction remains pending for more than 1 hour, and when it finally gets executed, the withdrawal period has ended, causing Alice's withdraw call to revert.

• As ongoingWithdrawals only allows withdrawals during the specified period, Alice will not be able to call withdraw again, resulting in the loss of all her airdropped tokens.

In both scenarios, users risk losing a portion or all of their airdropped tokens due to the unpredictability of transaction execution timestamps (block.timestamp). And this aren't exceptional scenarios as this is expected to happen to many other users because the tx execution timestamp block.timestamp is not the one in which the tx is submitted but rather the time of the block in which the tx is included which isn't always predictable as tx can stay pending in the mempool for many hours (or even days).

And the fact that the full airdropped amount is only withdrawable at a specific timestamp claimEnd + withdrawalWindow makes the chances of this issue occurring even more frequent as users will want to get the maximum amount of tokens and end up losing some of it

Impact

Users may lose a portion or all their airdropped amounts because of the withdrawable period.

Tools Used

Manual review, VS Code

Recommended Mitigation

One possible solution to address this issue is to remove the ongoingWithdrawals modifier from the withdraw function. This modification would allow users to withdraw their airdropped tokens at any time, mitigating the risk of loss due to transaction execution timing.

Assessed type

Context

[c4-pre-sort]

minhquanym marked the issue as duplicate of #245

[c4-judge]

0xean changed the severity to 3 (High Risk)

[c4-judge]

0xean marked the issue as satisfactory