You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Background contracts/SECURITY_REVIEW_MembershipNFT.md (finding #4) documents that MembershipNFT's entire security model rests on a single owner EOA and an admins mapping controlled solely by owner. The review added two-step ownership transfer (transferOwnership/acceptOwnership) as a zero-risk mitigation, but explicitly recommends — as unimplemented follow-up work — that owner be a multisig and that a timelock sit in front of setAdmin so a compromised owner key can't instantly grant itself/an attacker admin rights.
Problem
Even with two-step ownership transfer, a single compromised owner private key can instantly call setAdmin and grant minting/suspension rights to an attacker-controlled address, with no delay for the community to notice and react before damage (mass minting, mass suspension) occurs.
Expected outcome
A TimelockedAdmin mechanism (or integration with a standard timelock pattern) sits in front of setAdmin, requiring privileged admin-set operations to be queued and only executable after a configurable delay, with an AdminChangeQueued/AdminChangeExecuted/AdminChangeCancelled event trail for off-chain monitoring. This should be additive and not break the existing onlyAdmin gating used by mint/renew/setSuspended.
Suggested implementation
Design a minimal timelock specifically for setAdmin calls (rather than pulling in a full generic timelock/governance framework, to keep the supply-chain surface small, consistent with the review's stated preference for minimal, audited additions).
Add queueSetAdmin(address who, bool enabled), executeSetAdmin(address who, bool enabled) (callable only after the delay has elapsed and only by owner), and cancelSetAdmin(...).
Emit events for queue/execute/cancel so off-chain indexers (which, per the review, "trust these events completely") can alert communities of pending admin changes during the delay window.
Add Foundry tests in contracts/test/ mirroring the existing test style (MembershipNFT.t.sol conventions): queuing, executing after delay, executing before delay reverts, cancelling, double-execution reverts, and a fuzz test on delay boundary timing similar to the existing testFuzz_expiryBoundary pattern.
Update contracts/SECURITY_REVIEW_MembershipNFT.md to record this as resolving the residual risk noted in finding Add unit tests for the policy engine #4, and note that owner still being a multisig is a deployment/operational recommendation rather than a code change.
Acceptance criteria
setAdmin changes can only take effect through the queue → delay → execute flow (or an explicit, clearly-justified emergency path if one is added).
All new state transitions emit events sufficient for off-chain monitoring.
Foundry test suite covers queuing, delay enforcement, execution, cancellation, and edge/fuzz cases, following existing test conventions.
contracts/SECURITY_REVIEW_MembershipNFT.md is updated to reflect the change and its residual-risk status.
No existing mint/renew/setSuspended behavior or gas costs regress unexpectedly (documented if they do, consistent with the review's existing gas-impact reporting style).
Difficulty: Advanced
Type: Security / Feature (Solidity)
Background
contracts/SECURITY_REVIEW_MembershipNFT.md(finding #4) documents thatMembershipNFT's entire security model rests on a singleownerEOA and anadminsmapping controlled solely byowner. The review added two-step ownership transfer (transferOwnership/acceptOwnership) as a zero-risk mitigation, but explicitly recommends — as unimplemented follow-up work — thatownerbe a multisig and that a timelock sit in front ofsetAdminso a compromised owner key can't instantly grant itself/an attacker admin rights.Problem
Even with two-step ownership transfer, a single compromised
ownerprivate key can instantly callsetAdminand grant minting/suspension rights to an attacker-controlled address, with no delay for the community to notice and react before damage (mass minting, mass suspension) occurs.Expected outcome
A
TimelockedAdminmechanism (or integration with a standard timelock pattern) sits in front ofsetAdmin, requiring privileged admin-set operations to be queued and only executable after a configurable delay, with anAdminChangeQueued/AdminChangeExecuted/AdminChangeCancelledevent trail for off-chain monitoring. This should be additive and not break the existingonlyAdmingating used bymint/renew/setSuspended.Suggested implementation
setAdmincalls (rather than pulling in a full generic timelock/governance framework, to keep the supply-chain surface small, consistent with the review's stated preference for minimal, audited additions).queueSetAdmin(address who, bool enabled),executeSetAdmin(address who, bool enabled)(callable only after the delay has elapsed and only byowner), andcancelSetAdmin(...).contracts/test/mirroring the existing test style (MembershipNFT.t.solconventions): queuing, executing after delay, executing before delay reverts, cancelling, double-execution reverts, and a fuzz test on delay boundary timing similar to the existingtestFuzz_expiryBoundarypattern.contracts/SECURITY_REVIEW_MembershipNFT.mdto record this as resolving the residual risk noted in finding Add unit tests for the policy engine #4, and note thatownerstill being a multisig is a deployment/operational recommendation rather than a code change.Acceptance criteria
setAdminchanges can only take effect through the queue → delay → execute flow (or an explicit, clearly-justified emergency path if one is added).contracts/SECURITY_REVIEW_MembershipNFT.mdis updated to reflect the change and its residual-risk status.mint/renew/setSuspendedbehavior or gas costs regress unexpectedly (documented if they do, consistent with the review's existing gas-impact reporting style).Likely affected files/directories
contracts/src/MembershipNFT.sol,contracts/test/MembershipNFT.t.sol,contracts/test/MembershipFuzzInvariant.t.sol,contracts/SECURITY_REVIEW_MembershipNFT.md