Skip to content

refactor: contracts#2281

Open
MarcosNicolau wants to merge 3 commits intotestnetfrom
vesting-airdrop
Open

refactor: contracts#2281
MarcosNicolau wants to merge 3 commits intotestnetfrom
vesting-airdrop

Conversation

@MarcosNicolau
Copy link
Copy Markdown
Member

Summary

  • Encode validFrom in the Merkle leaf so each claim leaf represents a vesting stage gated by timestamp.
  • Add claimBatch entry point to claim multiple stages in one tx with a single transferFrom.
  • Replace mapping(address => bool) hasClaimed with per-stage tracking keyed by keccak256(claimer, leaf).
  • Extract _verifyAndMark internal helper shared by claim and claimBatch.

Notes

  • Breaking change to the claim ABI (claim now takes validFrom) and to the Merkle leaf format. Any off-chain tree builder must be updated before this is deployed.
  • Storage layout change: hasClaimed key type changes from address to bytes32. Safe for a fresh deploy; not an in-place upgrade.

Introduce per-stage claims via a validFrom timestamp encoded in the
Merkle leaf, and a claimBatch entry point that claims multiple stages
in a single transferFrom. Replace the per-address hasClaimed flag with
a per-stage key keccak256(claimer, leaf).
@MarcosNicolau MarcosNicolau changed the title feat: vesting stages and batch claiming in airdrop feat: update contracts Apr 13, 2026
@MarcosNicolau MarcosNicolau changed the title feat: update contracts refactor: contracts Apr 13, 2026
@JuArce
Copy link
Copy Markdown
Collaborator

JuArce commented Apr 14, 2026

/claude /codex

@github-actions
Copy link
Copy Markdown

Codex Code Review

Findings

  1. High - Upgrade storage/key semantic break can re-enable already-claimed accounts
    File: ClaimableAirdrop.sol, ClaimableAirdrop.sol, ClaimableAirdrop.sol
    hasClaimed changed from mapping(address => bool) to mapping(bytes32 => bool) and lookups now use claimKey = keccak256(abi.encode(msg.sender, leaf)).
    For an upgradeable proxy, this effectively changes how claimed state is addressed. Existing claimed flags written under the old address-based keyspace will not be recognized by the new logic, so users who claimed before upgrade can claim again if still present in the merkle root.

Actionable fix:

  • Preserve backward compatibility by keeping a legacy check (e.g., old address-claim state) during transition, or
  • Explicitly guarantee/prove this upgrade is only deployed before any claims happen on that proxy instance.

No other concrete security/bug/performance issues stood out in this diff.

)
);

bytes32 claimKey = keccak256(abi.encode(msg.sender, leaf));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: redundant msg.sender in claim key

leaf is already keccak256(bytes.concat(keccak256(abi.encode(msg.sender, amount, validFrom)))), so it uniquely identifies (claimer, amount, validFrom). Re-hashing msg.sender with it adds a wasted keccak256 call without extra security.

Suggested change
bytes32 claimKey = keccak256(abi.encode(msg.sender, leaf));
bytes32 claimKey = leaf;

require(success, "Failed to transfer funds");

emit TokensClaimed(msg.sender, amount);
emit TokensClaimed(msg.sender, totalClaimable);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: batch emits a single event, losing per-stage auditability

Off-chain indexers (and future auditors) can only see the total amount transferred, not which individual stages were claimed. Consider emitting one TokensClaimed event per stage inside _verifyAndMark, or adding a separate StagesClaimed event that logs the stage details.

@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

Review: ClaimableAirdrop vesting refactor

Overall the design is sound — per-stage claim tracking, nonReentrant on both entry points, and state written before any external call. Two inline findings plus one deployment hazard below.


Medium — in-place upgrade allows re-claims by existing claimants

hasClaimed changes its key type from address to bytes32. Both occupy the same storage slot but keys are computed differently:

  • Old: hasClaimed[msg.sender]
  • New: hasClaimed[keccak256(abi.encode(msg.sender, leaf))]

If this is deployed as an in-place upgrade of a live contract, every previously-claimed address looks unclaimed to the new code. Combined with an updated Merkle root they could claim again.

The PR description flags this, but the contract has no guard — no reinitializer that migrates or locks old entries, no revert in _authorizeUpgrade, no NatSpec warning on the storage variable. A misapplied upgrade would silently allow double-claims. At minimum add a /// @custom:storage-layout-breaking-change or equivalent NatSpec on the mapping so the hazard is visible in the source.


See inline comments for the two lower-severity findings.

The leaf already encodes msg.sender in its preimage, making the
extra keccak256(abi.encode(msg.sender, leaf)) wrapper redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants