feat: supports EIP-712 and EIP-1271 signatures to claim airdrop #160
feat: supports EIP-712 and EIP-1271 signatures to claim airdrop #160smol-ninja merged 13 commits intostagingfrom
Conversation
refactor: adds a shared function that reverts if `to` address is zero
9cbce83 to
7c415c1
Compare
andreivladbrg
left a comment
There was a problem hiding this comment.
Good job on this — it looks like you've put in effort. Overall, it looks good (I've only reviewed the src) , I’ve left a few comments:
Should we add an expiration to the signature? It could be <= campaign expiration.
| /// @param merkleProof The proof of inclusion in the Merkle tree. | ||
| function claimTo(uint256 index, address to, uint128 amount, bytes32[] calldata merkleProof) external payable; | ||
|
|
||
| /// @notice Claim airdrop on behalf of eligible recipient using an EIP-712 or EIP-1271 signature. |
There was a problem hiding this comment.
what if we mention (i) on behalf of eligible recipient (ii) sending it to to address
also: "the airdrop recipient must have signed the message before calling the function"
wdyt?
There was a problem hiding this comment.
Claim airdrop on behalf of eligible recipient using an EIP-712 or EIP-1271 signature, and transfer the
tokens to thetoaddress.
Is this good? (01acf19#diff-f6dbd13750414f75f19e46f764de18fcabedac202f2776bf73fb5c39cf83de39)
the airdrop recipient must have signed the message before calling the function
Its already added in the requirement as "If recipient is an EOA, it must match the recovered signer.". To match the recovered signer, it must have signed the msg. Wdyt?
|
Thanks for the review Andrei.
Whats the use case? Since the tx is public, anybody can still execute it. Also, after the campaign is expired, the signature wont work anyways. |
ik that if the campaign is expired no claims are possible, but the idea of having a "forever" signature doesn't sound right maybe (if its possible) someone reuses it in other contexts of attacks how i see it is that we minimize the attack surface externally |
|
re signatures on tests i remember facing lots of issues when i integrated don't know if it will help, but will leave it here: https://github.com/Uniswap/permit2/blob/cc56ad0f3439c502c246fc5cfcc3db92bb8b7219/test/utils/PermitSignature.sol |
I can't comment on "sounding right" but technically speaking, signature is useless anyway after campaign expiry. Also, it has replay protection so the same signature cannot be used on another chain or another campaign. I am happy to implement it if there is any UX advantage which I am not seeing right now.
But no attacks are possible because (1) each campaign has different address (2) chain ID is calculated in the constructor for hash The only attack is in case of a fork but in that case, the campaign will also be forked and all eligibility will be valid on both the chains. |
01acf19 to
0173076
Compare
* docs: update natspecs for claim functions * feat: adds claim-via-signature function refactor: adds a shared function that reverts if `to` address is zero * refactor: moves domain type hash inside constructor * tests: adds tests for claim via signature * docs: fix domain name * chore: adds AI recommendations * chore: update evm-utils version * test: default recipient functions * refactor: add notZeroAddress modifier * build: bump packages * chore: remove redundant code * docs: update contributing file * docs: polish natpsec --------- Co-authored-by: Andrei Vlad Birgaoanu <andreivladbrg@gmail.com>
Sorry for the massive PR @andreivladbrg. It was very complicated than I thought, both writing source code and then testing it. Now its ready for your review. I will recommend to read the PR description first.
This should be merged after the following PR:
Changelog
Source code
viewto read only functionsSignatureHashlibrary to keep signature hashes. This is a better approach as it keeps the campaign related constants separate from the signature related constants.Tests
users.recipientusingcreateUserAndKeyfunction as we need private key to create signatures.Basetest contract and Integration base contractgetIndexInMerkleTreeandgetMerkleProoffunctions replacing hardcoded use ofINDEX1andindex1Proofto avoid manual errorsclaimViaSigand ignored what is already being thoroughly tested throughclaimandclaimTo. This is also why I didn't write fuzz tests for it.Types.solfor details.Some notes
tests/utils/Utilities.solcontain signature utilities but note that it uses solidity whereas the correct approach is to useeth_signTypedData_v4so its not being tested. I will wait for foundry to release the new cheatcode for that. Nevertheless, using approach below, I have verified that signature are correctly generated.To generate EIP-712 signature, run the following in your browser:
Load your address:
Define "TypedData":
Call "eth_signTypedData_v4":
It will then show you something like this:
Once you sign it, it will then display the signature in the console.
Some reference: