Arcade.xyz - osmanozdemir1's results

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeAirdrop.sol#L75-L76 https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/libraries/ArcadeMerkleRewards.sol#L40

Vulnerability details

The governance can change the rewardsRoot in the ArcadeAirdrop.sol, and there are no restrictions while changing the root. The root can be changed even after the claim period has been started.
https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeAirdrop.sol#L75-L76

File: ArcadeAirdrop.sol 
   function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {
        rewardsRoot = _merkleRoot;
        emit SetMerkleRoot(_merkleRoot);
    }

When a user claims an airdrop, he gets the totalGrant in the current root, but the possible root change is not considered here.
The airdrop can be claimed only once regardless of the maximum claim amount in the root. In the case of a root change scenario where a user's totalGrant is greater in the new root than the old root, they can not claim the difference between the new root and the old root.

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/libraries/ArcadeMerkleRewards.sol#L106

File: ArcadeMerkleRewards.sol
106.    if (claimed[msg.sender] != 0) revert AA_AlreadyClaimed();

Impact

Users can not claim their claimable tokens in the case of airdrop root change, even if they have right to claim in the new root.

Proof of Concept

As I mentioned above, governance can change the airdrop Merkle root anytime, even after the claim period was started. There might be different reasons for this. It might be because a mistake was found in the previous root or the governance wanted to change the airdrop criteria etc.

Let's assume Alice's totalGrant was 100 in the root and the claim period started.

1. Alice claimed her airdrop and claimed[msg.sender] is updated.

2. A week later governance decided to change the root for some reasons like the above, and a new root has been set.

3. According to the new root, Alice's totalGrant is 500.

4. Alice wanted to claim the difference of 400 tokens.

5. Transaction failed because of this line, and Alice couldn't claim her right.

Tools Used

Manual review

Recommended Mitigation Steps

There might be a few ways to mitigate the issue. In the current implementation, the airdrop claims only have an expiration time and not a start time. The first way to mitigate the issue might be setting a start time for airdrop claims, and not letting governance change the root after the claim period starts. Setting a start time might give governance enough buffer period to make changes to the root. This mitigation way restricts the governance's power and I acknowledge that the protocol might not want to do that.

Another way to mitigate the issue is to check if the claimed amount is equal to totalGrant during validation. Right now it checks if the claimed amount is zero or not, and reverts if it is not zero. Reverting if the claimed amount is equal to totalGrant, and letting users claim the difference if the claimed amount is lower than the totalGrant would solve the issue. It can be implemented like this:

Validating:

File: ArcadeMerkleRewards.sol
+++105.   // ensure the user has not already claimed totalGrant
+++106.   if (claimed[msg.sender] == totalGrant) revert AA_AlreadyClaimed();
+++107.   claimed[msg.sender] = totalGrant;

Claiming:

File: ArcadeMerkleRewards.sol

+++     uint256 claimedAmount = claimed[msg.sender];
+++     uint256 unclaimedAmount = totalGrant - claimedAmount;

        // validate the withdraw
        _validateWithdraw(totalGrant, merkleProof);

+++     // Use the unclaimed amount in the calls below
+++     token.approve(address(votingVault), uint256(unclaimedAmount));
+++     votingVault.airdropReceive(msg.sender, unclaimedAmount, delegate);

This way, users can still claim the difference anytime the root changes.

Assessed type

Invalid Validation