Expiration in token airdrop is not inclusive of multiple airdrops and cannot be changed making contract unusable

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-arcade/blob/f8ac4e7c4fdea559b73d9dd5606f618d4e6c73cd/contracts/token/ArcadeToken.sol#L74-L171

Vulnerability details

Impact

The expiration time stamp set in the constructor is one time thing and there is no setter function to change and for multiple airdrops, initial expiration may become obsolete and even on changing the merkle root the airdrop cannot happen.

Proof of Concept

Consider the following scenerio:

1. Airdrop is decided the be happen.Contract deployed all params set, expiration is 1 month from now.

2. Merkle root is set by the owner for the beneficiaries to claim the airdrop using the following functions:

   function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {
       rewardsRoot = _merkleRoot;
   
       emit SetMerkleRoot(_merkleRoot);
   }

3. Now users can claim the airdrop using the the following function from ArcadeMerkleRewards.sol:

   function claimAndDelegate(address delegate, uint128 totalGrant, bytes32[] calldata merkleProof) external {
       // must be before the expiration time
       if (block.timestamp > expiration) revert AA_ClaimingExpired();
       // no delegating to zero address
       if (delegate == address(0)) revert AA_ZeroAddress("delegate");
       // validate the withdraw
       _validateWithdraw(totalGrant, merkleProof);
   
       // approve the voting vault to transfer tokens
       token.approve(address(votingVault), uint256(totalGrant));
       // deposit tokens in voting vault for this msg.sender and delegate
       votingVault.airdropReceive(msg.sender, totalGrant, delegate);
   }

   Reward claimed and delegated. All good for the first iteration. One month has passed.

But lets say now arcade wants to do the airdrop again to same or different set of addresses. Merkle root is set with generated from the beneficiaries data (address, claimable amount) etc. Ideally this should have worked but the catch is expiration have passed and cannot be changed. So the this contract becomes useless now leading the team to do the costly redeploy and transactions once again.

Tools Used

Manual review

Recommended Mitigation Steps

Make a setter function for the expiration to be able to make multiple airdrops over different period of time, each with its own expiration.

Assessed type

DoS

[141345]

The airdrop might not be intended for repeat use.

[c4-sponsor]

PowVT marked the issue as sponsor confirmed

[c4-sponsor]

PowVT marked the issue as disagree with severity

[PowVT]

QA: duplicate of #231 . Will be fixed.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a

[Nabeel-javaid]

IMO this is a valid medium for following reasons:

1. As the issue #213 being confirmed by sponsor clearly suggests that there could be multiple airdrops over period of time not one, but there is no ability to change the expiration date that is not intentional and make the contract simply unusable.

2. And if the argument is that another contract can be deployed that stays true for #213 too, which is not an optimal solution and clearly not intentional from the protocol.

Will appreciate further comments.

[0xean]

This seems different than either of those 2 issue as its really comes down to input sanitization. The expiration date could be very far in the future. You are arguing for making this mutable vs immutable, which is a design choice.

The sponsor clearly intends to update the claimable values since they confirmed those other issues, so the code not allowing for that is problematic. The sponsor could still set a expiration far enough in the future to facilitate additional rewards.

[Nabeel-javaid]

Maybe it can be reassured from the sponser again, because even docs mentioned that manager can update the expiration, so it is not even in line with the docs too. Secondly setting expiration far off in the future means the current batch does not even have any expiration it than becomes expiration for overall batches, which is clearly not intended here.

So the design does not seems intended to me, as it does not work as expected and is not in line with the c4 detail page either.

@PowVT, will really appreciate you feedback here based on above comments.

[PowVT]

We made the fix for #231 and this seems along the same lines to me. Is QA since like the judge said there is an easy work around and is mostly input sanitization.

[Nabeel-javaid]

Right but how its input sanitization when work around is not even the actual fix.

If the batch of airdrop is run, and have certain expiration. Based on the info that there would be further airdrop batches. How can you set the expiration for first batch.

Lets say you set the expiration too far ahead, that will not be the expiration for the initial batch. So clearly it shows that such input sanitization will not work.

Having said that, upto the judge but from the discussion to me it does not seem like a QA or simple input sanitation thing,