Review: Check merkeproof is set in claimTokens()

[gpersoon]

Impact

The merkeproof is set at a later moment in time via setMerkleRoot()
If claimTokens() would be used before the merkeproof is set, it wouldn't work
(although this is very unlikely in practice)

Proof of Concept

contracts/contracts/ArenaToken.sol

Lines 74 to 81 in 4a86c18

	function claimTokens(uint256 amount, bytes32[] calldata merkleProof) external {
	require(block.timestamp < claimPeriodEnds, "ArenaToken: Claim period ended");
	// we don't need to check that `merkleProof` has the correct length as
	// submitting a valid partial merkle proof would require `leaf` to map
	// to an intermediate hash in the merkle tree but `leaf` uses msg.sender
	// which is 20 bytes instead of 32 bytes and can't be chosen arbitrarily
	bytes32 leaf = keccak256(abi.encodePacked(msg.sender, amount));
	(bool valid, uint256 index) = MerkleProof.verify(merkleProof, merkleRoot, leaf);

contracts/contracts/ArenaToken.sol

Lines 134 to 138 in 4a86c18

	function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {
	require(merkleRoot == bytes32(0), "ArenaToken: Merkle root already set");
	merkleRoot = _merkleRoot;
	emit MerkleRootChanged(_merkleRoot);
	}

Tools Used

Recommended Mitigation Steps

Check merkeproof != 0 claimTokens()

[HickupHH3]

hmmm IMO this isn't necessary since claim attempts will already revert as it is. The only benefit is that it will cause the revert to happen earlier, with the downside being a bit more gas for successful claim attempts. Optimizing for the latter would be of greater benefit

[gpersoon]

No problem to leave it as is, however we are on a chain with low gas fees (polygon) so saving gas isn't that important.