Replace drop authorization with EIP-712#56
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughImplements EIP-712 DropAuthorization in StreamDrops: signature-based minting (EOA + EIP-2098), domain-separated hashing, consumed/cancelled drop storage, signer-epoch rotation, admin epoch/cancel controls, removal of legacy packed helper, and extensive test/integration migrations validating positive and negative EIP-712 flows. ChangesEIP-712 Drop Authorization
Sequence Diagram(s)sequenceDiagram
participant User
participant StreamDrops
participant ECDSA
participant MockStreamMinter
User->>StreamDrops: call mintDrop(authorization, tokenData, signature)
StreamDrops->>ECDSA: compute EIP-712 digest, recover signer
ECDSA-->>StreamDrops: recovered EOA
StreamDrops->>StreamDrops: validate authorization, mark drop consumed
StreamDrops->>MockStreamMinter: execute fixed-price or auction mint
MockStreamMinter-->>StreamDrops: return mint result
StreamDrops-->>User: emit events / return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@claude please review this PR for correctness and security. Focus areas:
|
|
@coderabbitai review The PR status has been pending on the CodeRabbit processing note since 07:11 UTC. Please complete or refresh the review for the latest head commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all relevant Solidity files
fd -e sol --full-path | head -40
echo "---"
# Read main contract
fd "StreamDrops.sol" --exec cat {}Length of output: 17679 🏁 Script executed: #!/bin/bash
# Read EIP-712 test file
fd "StreamDropsEIP712.t.sol" --exec cat {}Length of output: 15142 🏁 Script executed: #!/bin/bash
# Read test helpers
fd "DropAuthTestHelper.sol" --exec cat {}
fd "CharacterizationTestBase.sol" --exec cat {}Length of output: 5155 🏁 Script executed: #!/bin/bash
# Read characterization and integration test files
fd "StreamDropsCharacterization.t.sol" --exec cat {}
fd "StreamDropsIntegrationCharacterization.t.sol" --exec cat {}Length of output: 23816 🏁 Script executed: #!/bin/bash
# Check for any remaining packed-hash / old surface
rg -n "retrieveMessageAndDropID|mintDrop\(address,address,string|dropExecuted|MintsToTxOrigin|tx\.origin|abi\.encodePacked" --type sol
echo "---"
# Check for old mintDrop signature (7 params)
rg -n "mintDrop" --type solLength of output: 586 🏁 Script executed: #!/bin/bash
# Read ADR and roadmap docs
fd "ADR" | head -10
fd "ROADMAP" | head -10
fd "BLOCKERS" | head -10
fd -e md | head -20
echo "---"
cat README.md 2>/dev/null || trueLength of output: 3102 🏁 Script executed: #!/bin/bash
cat docs/adr/0001-drop-authorization.md
echo "==="
cat docs/known-blockers.md
echo "==="
cat ops/ROADMAP.mdLength of output: 102659 🏁 Script executed: #!/bin/bash
# Check for old packed-hash surface and mintDrop usage
rg -n "retrieveMessageAndDropID|mintDrop|dropExecuted|MintsToTxOrigin|tx\.origin|encodePacked" smart-contracts/ test/
echo "---"
# Verify DROP_AUTHORIZATION_TYPEHASH string matches struct fields
rg -n "DROP_AUTHORIZATION_TYPEHASH\|DropAuthorization\|EIP712_DOMAIN_TYPEHASH\|DROP_ID_TYPEHASH" smart-contracts/StreamDrops.solLength of output: 14395 🏁 Script executed: #!/bin/bash
# Read ADR docs and blockers
cat docs/adr/0001-drop-authorization.md 2>/dev/null || echo "not found"
cat docs/known-blockers.md 2>/dev/null || echo "not found"
# Check IStreamDrops interface
cat smart-contracts/IStreamDrops.solLength of output: 18637
EIP-712 Domain and
|
| Check | Result |
|---|---|
65-byte ECDSA: r | s | v via calldataload |
✓ correct offsets |
64-byte EIP-2098: r | vs, s = vs & 0x7fff…, v = (vs >> 255) + 27 |
✓ |
Low-s guard: uint256(s) <= SECP256K1_N_DIV_2 |
✓ |
Valid-v guard: v == 27 || v == 28 |
✓ |
| Zero-recovered-signer guard | ✓ |
Wrong signer (_signer == tdhSigner) |
✓ |
Stale epoch (signerEpoch equality) |
✓ |
Expired deadline (>= block.timestamp) |
✓ |
Zero dropId |
✓ |
dropId derivation recomputed from validated signer + epoch + nonce + salt |
✓ |
| Consumed state | ✓ |
| Cancelled state | ✓ |
Payer policy (paid: payer == msg.sender; free: payer == address(0)) |
✓ |
Tests cover: wrong signer, wrong verifying contract, wrong chain ID, expired deadline, cancelled, stale epoch, wrong dropId, quantity != 1, token-data substitution, malleable, zero-recovered-signer, invalid length, paid-payer mismatch, contract signer. ✓
ERC-1271 Boundary
require(tdhSigner.code.length == 0, "ERC1271 pending");Checked first, before any signature processing. testContractSignerIsExplicitlyRejectedUntilErc1271Lands deploys a ContractSignerStub and confirms the call reverts. The boundary is clean and correctly documented as pending #19. ✓
Old Packed-Hash Surface
Confirmed absent from smart-contracts/StreamDrops.sol:
retrieveMessageAndDropID— removed ✓dropExecuted— removed ✓tx.origin— removed ✓- Old 7-parameter
mintDropABI — removed ✓ - The one
abi.encodePackedat Line 291 is the standard"\x19\x01"EIP-712 envelope prefix, which is correct by spec ✓
The abi.encodePacked occurrences in NFTdelegation.sol, StreamCuratorsPool.sol, XRandoms.sol, etc. are pre-existing and tracked under their own Slither rows — none are in the drop authorization path. ✓
Issues Found
1. cancelDrop emits duplicate events without idempotency guard
function cancelDrop(bytes32 _dropId) public FunctionAdminRequired(this.cancelDrop.selector) {
require(consumedDropIds[_dropId] == false, "Drop consumed");
cancelledDropIds[_dropId] = true; // idempotent state
emit DropAuthorizationCancelled(_dropId, msg.sender); // but event fires again
}There is no require(cancelledDropIds[_dropId] == false, ...) guard. Calling cancelDrop twice on the same dropId passes the consumed check, writes the same boolean (no-op), but emits DropAuthorizationCancelled twice. Off-chain indexers treating each event as a distinct cancellation will double-count. Suggested fix:
function cancelDrop(bytes32 _dropId) public FunctionAdminRequired(this.cancelDrop.selector) {
require(consumedDropIds[_dropId] == false, "Drop consumed");
require(cancelledDropIds[_dropId] == false, "Already cancelled");
cancelledDropIds[_dropId] = true;
emit DropAuthorizationCancelled(_dropId, msg.sender);
}2. Five test cases from ADR acceptance criteria are missing
The ADR §Test Plan lists these as required; none appear in StreamDropsEIP712.t.sol or the characterization files:
| Missing test | Code path exercised |
|---|---|
Free fixed-price where payer != address(0) |
require(_authorization.payer == address(0), "payer") in the price == 0 branch |
Auction where payer != address(0) |
require(_authorization.payer == address(0), "payer") in auction branch |
Fixed-price with auctionReservePrice != 0 |
require(_authorization.auctionReservePrice == 0, "Auction price") |
Fixed-price with auctionEndTime != 0 |
require(_authorization.auctionEndTime == 0, "Auction end") |
Auction with price != 0 |
require(_authorization.price == 0, "Fixed price") |
These are simple require paths and the production behavior is correct, but the ADR acceptance criteria explicitly lists them and they should be added for Gate C completeness.
3. No event content assertions
The ADR requires event assertions for DropAuthorizationConsumed, DropAuthorizationCancelled, and SignerEpochChanged. The tests only assert isDropConsumed storage state and isDropCancelled state — they do not verify event emission or field content. These are also part of the ADR acceptance criteria for this issue.
Pre-existing Issues (Out of Scope for This PR)
- ETH dust from integer division in
_executeFixedPriceDrop(msg.value / 2,/ 4,/ 4) — anymsg.valuenot divisible by 4 traps wei in the contract. Tracked under P0-PAY-001. - Push payment DoS — a reverting poster,
payOutAddress, orcuratorsPoolAddressreverts the entire mint and the drop is not consumed (characterization tests correctly document this). Tracked under P0-PAY-001.
Test and Documentation Traceability for #10
- ADR 0001
## Implementation Statussection is updated to reflect the P0-AUTH-002 state. ✓ - Roadmap
P0-AUTH-002entry updated with current status. ✓ ops/ROADMAP.mdAppendix B test matrix row forAd hoc drop authorizationupdated toPassing for EOA/EIP-2098 target state. ✓- Slither baseline updated:
encode-packed-collisionforStreamDrops.mintDropandretrieveMessageAndDropIDremoved;uninitialized-localforStreamDrops.mintDropmarked Fixed. ✓ docs/known-blockers.mdupdated to describe the current EIP-712 EOA state and the ERC-1271 blocker. ✓
The three findings above (duplicate-cancel event, 5 missing negative tests, missing event assertions) are the only gaps relative to the ADR acceptance criteria. Everything in the cryptographic and replay-protection layer is correct.
|
Follow-up verification before merge:
|
1 participant
Summary
StreamDrops.mintDroppath with EIP-712DropAuthorizationtyped data.s, valid-v, and zero-recovered-signer checks.P0-AUTH-003/ [P0-AUTH-003] Implement ERC-1271 contract signer support #19.retrieveMessageAndDropIDand the oldmintDrop(address,address,string,uint256,uint256,uint256,uint256)execution surface.Closes #10.
Review Follow-up
Already cancelledguard socancelDropcannot emit duplicate cancellation events.Validation
forge test --match-contract StreamDropsEIP712Test -vvvpassed with 23 tests.forge test --match-contract StreamAdminsTest -vvvpassed with 4 tests.make checkpassed with 47 tests and the known Solidity warning baseline.powershell -ExecutionPolicy Bypass -File scripts\check.ps1passed with 47 tests and the known Solidity warning baseline.forge fmt --check smart-contracts\StreamDrops.sol test\StreamAdmins.t.sol test\StreamDropsCharacterization.t.sol test\StreamDropsIntegrationCharacterization.t.sol test\StreamDropsEIP712.t.sol test\helpers\CharacterizationTestBase.sol test\helpers\DropAuthTestHelper.solpassed.git diff --checkandgit diff --cached --checkpassed.rg -n "retrieveMessageAndDropID|mintDrop\\(address,address,string|dropExecuted|MintsToTxOrigin|tx\\.origin" smart-contracts testreturned no matches.make slitherreturned non-zero as expected because unrelated baseline findings remain; targeted log check confirmed the oldStreamDropspacked-hash findings are gone, onlyStreamCore.retrieveDependencyScriptremains underencode-packed-collision, andStreamDrops.mintDropno longer appears underuninitialized-local.Notes
Summary by CodeRabbit
New Features
Documentation
Tests