Repository navigation
fix(escrow,trampoline): address V12 audit findings (COW-1272) - #40
Merged
jean-neiverth merged 9 commits intoAug 21, 2026
Merged
jean-neiverth merged 9 commits into
jean-neiverth merged 9 commits into
Conversation
The _sellToken parameter was already unnamed in the implementation and served no purpose. Removing it simplifies the interface and reduces calldata size. Addresses V12 audit finding F-234211/F-234212 cleanup.
Reject cooldown values exceeding 30 days in both the constructor and setCooldownPeriod to prevent overflow in the withdrawal timestamp arithmetic (withdrawalRequestedAt + cooldownPeriod). Addresses V12 audit finding F-234217.
Override renounceRole to revert for OPERATOR_ROLE and DEFAULT_ADMIN_ROLE when the contract is paused, preventing a permanently stuck pause that would lock user funds. Addresses V12 audit finding F-234221.
Reject admin transfer delays exceeding type(uint48).max / 2 to prevent the OZ AccessControlDefaultAdminRules timestamp arithmetic from overflowing, which would permanently disable admin rotation. Addresses V12 audit finding F-234109.
…COW-1272) Add both trade token addresses to PROPOSAL_TYPEHASH and the Proposal struct so signatures cannot be reused with an unrelated measurement token. The separate _buyToken execute() parameter is removed — _proposal.buyToken is used for the balance-delta check instead.
yvesfracari
approved these changes
Aug 21, 2026
yvesfracari
left a comment
yvesfracari
left a comment
Contributor
There was a problem hiding this comment.
Looks good overall. Please address the inline findings in this PR.
| string memory _name, | ||
| string memory _symbol | ||
| ) ERC20(_name, _symbol) AccessControlDefaultAdminRules(_adminTransferDelay, _admin) { | ||
| if (_adminTransferDelay > type(uint48).max / 2) revert Escrow_InvalidAdminTransferDelay(); |
Contributor
There was a problem hiding this comment.
would - Could changeDefaultAdminDelay bypass this constructor-only check? A near-max delay may overflow OZ's scheduling arithmetic.
- Add sellToken/buyToken signature-binding tests (comment 2) - Add cooldown period boundary tests for constructor and setter (comment 3) - Add paused renounceRole tests for protected and allowed roles (comment 4) - Fix "seven fields" → "nine fields" in PROPOSAL_TYPEHASH doc comment (comment 5)
jean-neiverth
deleted the
jean/cow-1272-address-v12-audit-findings-for-escrow-and-trampoline
branch
August 21, 2026 16:54
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
2 participants
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
Addresses the actionable findings from the V12 AI audit, after filtering out noise (see COW-1272 for triage rationale).
sellTokenandbuyTokeninto the EIP-712 signature — both trade tokens are now signed proposal fields (nine-fieldProposalDatastruct), preventing a signature from being reused with an unrelated token pair (F-234212). The separate_sellToken/_buyTokencalldata parameters are removed fromexecute().cooldownPeriodto 30 days — prevents overflow inwithdrawalRequestedAt + cooldownPeriodarithmetic (F-234217)OPERATOR_ROLE/DEFAULT_ADMIN_ROLErenunciation while paused — prevents permanently stuck pause that would lock user funds (F-234221)_adminTransferDelay— rejects values >type(uint48).max / 2to prevent OZ timestamp overflow that disables admin rotation (F-234109)One finding deferred for separate work:
minBuyAmount > 0(F-234211)Test plan
forge fmt --checkcleancooldownPeriod(constructor + setter, at limit and limit+1)renounceRoletested for both protected roles and one allowed casesellToken/buyTokeneach reverts withTrampoline_InvalidSignature)