fix: close the audit's informational items in one pass - #15
Merged
Conversation
MinterControl: an address is a Controller or a Minter, never both, in any pairing. The direct form (a key over itself) was already refused; the indirect form -- A manages B while B manages A -- let each key refill the budget the other spends, the same failure as one key holding both ends. Refused as AddressAlreadyPaired at scheduling and again at execution. Chains are refused too: simpler than a graph walk, and a chain still concentrates authority. removeController leaving pending appointments in place is documented with its reason (a by-Minter index would be a set, for a case that is not inconsistent state) and the operating procedure (scan ControllerScheduled for the freed Minter). UpgradeControl.upgradeToAndCall refuses ether: it is payable only because the function it overrides is, and the proxy has no way to send value back out. Eip2612 documents the standard permit front-run property and the try/catch an integrator should use. Tests added for the gaps the audit listed -- ERC-1271 rejections, zero permit owner, EIP-3009 window endpoints, permit under a pause, seizure against a shrunk balance, cancelling a lapsed seizure, Minter re-activation, the revived-appointment case, the bare implementation refusing an upgrade -- plus two symbolic properties for the allowance-raise boundary. The ERC-1271 mock now answers a bad signature with the failure value instead of reverting. docs/security-review.md gains residual risks 4 and 5 and the new counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Why
Security audit Info items I-1 … I-8 and the listed test gaps, bundled as agreed.
What
Code
MinterControl— an address is a Controller or a Minter, never both, in any pairing. The indirect form of self-management (A manages B, B manages A) let each key refill the budget the other spends; now refused asAddressAlreadyPairedat scheduling and at execution. Chains (A→B→C) are refused too — simpler than a graph walk, and a chain still concentrates authority. Cost: two cold SLOADs per schedule/execute.UpgradeControl—upgradeToAndCallrefuses ether withValueNotAccepted. It ispayableonly because the overridden function is; the proxy could never return value.MockSmartAccount— answers a bad signature with0xffffffffviatryRecoverinstead of reverting, as a real ERC-1271 account does.Docs
removeControllerleaving pending appointments in place: documented in the module, ADR-003 anddocs/deploying.md, with the reason (a by-Minter index would be a set with removal on every schedule/execute/cancel, for a case that is not inconsistent state) and the procedure (scanControllerScheduledfor the freed Minter —pendingControlleris keyed by Controller).Eip2612header: the standard permit front-run property and the integrator's try/catch.docs/security-review.md: MinterControl invariant restated, residual risks 4 (pending revival) and 5 (permit front-run), counts updated.Tests (+15 forge, +2 Halmos)
ERC-1271 rejections (disowning account, non-ERC-1271 code) for both
permitand EIP-3009 · zeropermitowner · EIP-3009 window endpoints excluded ·permitunder a pause (raise blocked, lowering allowed, nonce untouched) · seizure against a shrunk balance keeps its schedule · lapsed seizure still cancellable · Minter re-activation · revived appointment afterremoveController· bare implementation refuses an upgrade · ether refused · unknown upgrade cancel reverts · role-conflict refusals both directions · symbolic: only raises are gated under a pause / a listing.Checks
npm run check— 136 tests (8 suites) · Slither 0 findings · Halmos runs in CI (21 properties)🤖 Generated with Claude Code