Remove tx.origin from drop execution#55
Merged
Merged
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.
📝 WalkthroughWalkthroughRemoves tx.origin from drop execution by adding explicit _recipient to mintDrop and retrieveMessageAndDropID; updates ADR, roadmap, ops state, StreamDrops logic, test helpers, characterization/integration tests, and Slither/selector/documentation baselines to validate recipient-based execution and drop-id construction. ChangesP0-AUTH-001: Remove tx.origin and Add Explicit Recipient
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@claude please review this PR. Focus areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ops/AUTONOMOUS_RUN.md`:
- Around line 992-997: Update the section heading "PR TBD: Remove `tx.origin`
from drop execution (Queue Item 14)" to include the actual PR number 55 instead
of "TBD" (e.g., "PR `#55`: Remove `tx.origin` from drop execution (Queue Item
14)"); locate the header text in AUTONOMOUS_RUN.md that currently contains the
branch name `codex/remove-tx-origin-drop-execution` or the pull request URL
`https://github.com/6529-Collections/6529Stream/pull/55` and replace "PR TBD"
with the concrete "PR `#55`".
In `@smart-contracts/StreamDrops.sol`:
- Around line 81-89: The mintDrop function currently allows a zero-address
_poster which can cause ETH to be sent to address(0) and break auction fallback
logic; add a require/assert to validate _poster is not address(0) at the start
of mintDrop and apply the same non-zero check to the other related minting
function(s) in this file (the second mintDrop overload/variant around lines
108-137) so any incoming _poster argument is rejected when zero before
proceeding with price/auction logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f72c8f-c8ce-4da0-82b4-ed5d4c8e14f4
📒 Files selected for processing (13)
docs/adr/0001-drop-authorization.mddocs/known-blockers.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdops/SLITHER_BASELINE.mdsmart-contracts/IArrngController.solsmart-contracts/StreamDrops.soltest/README.mdtest/StreamAdmins.t.soltest/StreamDropsCharacterization.t.soltest/StreamDropsIntegrationCharacterization.t.soltest/helpers/CharacterizationTestBase.soltest/helpers/StreamFixture.sol
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
smart-contracts/StreamDrops.sol (1)
132-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject ETH on the auction path.
This branch still accepts
msg.value, but it never forwards or refunds it. Any ETH sent with an auction mint stays trapped inStreamDrops, and this file does not expose a withdrawal path.Suggested fix
} else if (_opt == 2) { + require(msg.value == 0, "Auction value"); require(recipient == address(0), "Auction recipient"); tokenid = minterContract.mintAndAuction(payOutAddress, tokData, 0, colID, _endDate); posterAuctionAddress[tokenid] = poster; auctionPrice[tokenid] = _price; executionAddress = poster;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smart-contracts/StreamDrops.sol` around lines 132 - 137, The auction branch (_opt == 2) currently accepts msg.value but never forwards or refunds it, trapping ETH in StreamDrops; update the branch handling in the minting function to reject any ETH by adding a require(msg.value == 0, "Do not send ETH for auction") at the start of the _opt == 2 branch (near the call to minterContract.mintAndAuction) so no ETH can be sent accidentally; reference symbols: StreamDrops minting function branch for _opt == 2, minterContract.mintAndAuction, posterAuctionAddress, auctionPrice, executionAddress.
🧹 Nitpick comments (1)
smart-contracts/StreamDrops.sol (1)
91-105: ⚡ Quick winDeduplicate the drop payload encoding.
The auth payload is assembled twice by hand. These paths need to stay byte-for-byte identical, so a later field change can silently break signing if only one side gets updated.
Refactor sketch
+ function _buildDropMessage( + address _poster, + address _recipient, + string memory _tokenData, + uint256 _collectionID, + uint256 _opt, + uint256 _price, + uint256 _endDate + ) internal pure returns (string memory) { + return string( + abi.encodePacked( + Strings.toHexString(uint256(uint160(_poster)), 20), + Strings.toHexString(uint256(uint160(_recipient)), 20), + _tokenData, + _collectionID.toString(), + _opt.toString(), + _price.toString(), + _endDate.toString() + ) + ); + } + function mintDrop( address _poster, address _recipient, string memory _tokenData, uint256 _collectionID, @@ ) public payable authorized { require(_poster != address(0), "Zero poster"); - bytes32 dropId = keccak256( - abi.encodePacked( - string( - abi.encodePacked( - Strings.toHexString(uint256(uint160(_poster)), 20), - Strings.toHexString(uint256(uint160(_recipient)), 20), - _tokenData, - _collectionID.toString(), - _opt.toString(), - _price.toString(), - _endDate.toString() - ) - ) - ) - ); + string memory message = _buildDropMessage( + _poster, + _recipient, + _tokenData, + _collectionID, + _opt, + _price, + _endDate + ); + bytes32 dropId = keccak256(abi.encodePacked(message)); @@ ) public pure returns (string memory, bytes32) { - string memory message = string( - abi.encodePacked( - Strings.toHexString(uint256(uint160(_poster)), 20), - Strings.toHexString(uint256(uint160(_recipient)), 20), - _tokenData, - _collectionID.toString(), - _opt.toString(), - _price.toString(), - _endDate.toString() - ) - ); + string memory message = _buildDropMessage( + _poster, + _recipient, + _tokenData, + _collectionID, + _opt, + _price, + _endDate + ); bytes32 hashedMessage = keccak256(abi.encodePacked(message)); return (message, hashedMessage); }Also applies to: 236-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@smart-contracts/StreamDrops.sol` around lines 91 - 105, The drop payload encoding is duplicated (used to compute dropId and later for auth) which risks mismatches; create a single helper (e.g., buildDropPayload or encodeDropPayload) that takes _poster, _recipient, _tokenData, _collectionID, _opt, _price, _endDate and returns the exact bytes used for signing (using abi.encodePacked with the same string/hex conversions), then replace both the keccak256(abi.encodePacked(...)) call that computes dropId and the other auth payload assembly (lines ~236-257) to call that helper and reuse its result so both sides remain byte-identical. Ensure the helper is internal/pure and used in both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@smart-contracts/StreamDrops.sol`:
- Around line 132-137: The auction branch (_opt == 2) currently accepts
msg.value but never forwards or refunds it, trapping ETH in StreamDrops; update
the branch handling in the minting function to reject any ETH by adding a
require(msg.value == 0, "Do not send ETH for auction") at the start of the _opt
== 2 branch (near the call to minterContract.mintAndAuction) so no ETH can be
sent accidentally; reference symbols: StreamDrops minting function branch for
_opt == 2, minterContract.mintAndAuction, posterAuctionAddress, auctionPrice,
executionAddress.
---
Nitpick comments:
In `@smart-contracts/StreamDrops.sol`:
- Around line 91-105: The drop payload encoding is duplicated (used to compute
dropId and later for auth) which risks mismatches; create a single helper (e.g.,
buildDropPayload or encodeDropPayload) that takes _poster, _recipient,
_tokenData, _collectionID, _opt, _price, _endDate and returns the exact bytes
used for signing (using abi.encodePacked with the same string/hex conversions),
then replace both the keccak256(abi.encodePacked(...)) call that computes dropId
and the other auth payload assembly (lines ~236-257) to call that helper and
reuse its result so both sides remain byte-identical. Ensure the helper is
internal/pure and used in both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f86c270e-a168-4701-a2c8-e92adcc92bb1
📒 Files selected for processing (3)
ops/AUTONOMOUS_RUN.mdsmart-contracts/StreamDrops.soltest/StreamDropsCharacterization.t.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- test/StreamDropsCharacterization.t.sol
This was referenced Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tx.originusage fromStreamDropsdrop execution._recipientreserved asaddress(0), rejects non-zero auction recipients, rejects zero posters, and stores the poster as the current no-bid execution fallback so settlement does not target zero.Closes #18.
Notes
This intentionally does not implement the full EIP-712 authorization schema, signature replay model, or field-substitution guarantees. Those remain assigned to
P0-AUTH-002/ issue #10.Validation
forge test --match-contract StreamDropsCharacterizationTest -vvv(9 tests)forge test --match-contract StreamDropsIntegrationCharacterizationTest -vvv(10 tests)make check(24 tests)powershell -ExecutionPolicy Bypass -File scripts\check.ps1(24 tests)forge fmt --checkfor formatted touched Solidity files, preserving existing provider-interface style to avoid unrelated churngit diff --checkrg -n "tx\.origin" smart-contractsreturned no matches