Update governorMills with settable vote delay, period and queue period

[08xmt]

Makes votingDelay and votingPeriod setable variables and adds a new variable queuePeriod which limits how long a proposal can remain unqueued before expiring.

[claude]

⚠️ Code review skipped — your organization's overage spend limit has been reached.

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, reopen this pull request to trigger a review.

[08xmt]

N E M E S I S — Verified Findings

Scope

• Language: Solidity 0.5.16
• Contracts analyzed: contracts/token/GovernorMills.sol, contracts/token/Guardian.sol
• Related contract (read-only context): contracts/token/Timelock.sol
• Branch: fix/GovernorMills-update (HEAD 1f0e325 — "Integrate Guardian contract in GovernorMills")
• Comparison baseline: c6c9d68 — last mainnet-deployed version (had guardian = msg.sender as an EOA)
• Nemesis passes executed: 4 (Pass 1 Feynman → Pass 2 State → Pass 3 Feynman → Pass 4 State, converged)

---

Nemesis Map — Critical Cross-References

Function	Coupled state(s) touched	Sync status	Pass that surfaced it
GovernorMills.setGuardian	guardian ↔ Guardian-contract-callable functions	GAP — bricked	Pass 1 (Feynman) → Pass 2 confirmed unreachable
GovernorMills.acceptAdmin	guardian ↔ Timelock.pendingAdmin	GAP — bricked	Pass 1 (Feynman) → Pass 2 confirmed unreachable
state()	live queuePeriod / quorumVotes ↔ historical proposal	GAP — live read	Pass 2 (State) → Pass 3 confirmed exploit path
cancel()	proposer's CURRENT xinv balance ↔ OLD locked rate	MIXED reference frame	Pass 1 (Feynman ordering) → Pass 2 (State)
Guardian.executeRevertToPreviousGovernance	revertDeadline ↔ execute path	GAP — only queue checks deadline	Pass 1 (Feynman consistency)
Guardian.executeCancel	cancellableProposals[id] ↔ post-cancel reset	No reset after use	Pass 2 (State)

---

Verification Summary

ID	Source	Severity	Verdict
NM-001	Feynman P1 → State P2 cross-feed	HIGH	TRUE POSITIVE
NM-002	Feynman P1 → State P2 cross-feed	HIGH	TRUE POSITIVE
NM-003	State P2 → Feynman P3	MEDIUM	TRUE POSITIVE
NM-004	Feynman P1	MEDIUM	TRUE POSITIVE
NM-005	Feynman P1 → State P2	LOW	TRUE POSITIVE
NM-006	Feynman P1	LOW	TRUE POSITIVE
NM-007	State P2	LOW	TRUE POSITIVE
NM-008	Feynman P1	INFO	TRUE POSITIVE
NM-009	Feynman P1 (boundary)	INFO	TRUE POSITIVE
NM-010	Feynman P1 (consistency)	INFO	TRUE POSITIVE

---

VERIFIED FINDINGS

NM-001 — GovernorMills.acceptAdmin() is permanently unreachable

Severity: HIGH
Source: Feynman P1 ("WHY does this guard exist if no caller can satisfy it?") → State P2 (Mutation Matrix showed zero writers to a state that gates it)
Verification: Code trace — Hybrid (every Guardian outbound call enumerated)

Coupled cross-contract pair: GovernorMills.guardian ↔ {functions on guardian that can call GovernorMills.acceptAdmin}.
Invariant: The address stored in guardian must be capable of invoking acceptAdmin.

Feynman question that exposed it:

"Line 389: require(msg.sender == guardian, ...). The constructor pins guardian = address(new Guardian(msg.sender)). Which call path makes msg.sender == address(Guardian) here? Enumerate every external call Guardian makes."

State Mapper enumeration:
Guardian.sol makes exactly three outbound calls to governorMills (Guardian.sol:38,44,57):

• __queueSetTimelockPendingAdmin(oldGovernor, eta)
• __executeSetTimelockPendingAdmin(oldGovernor, eta)
• cancel(proposalId)

There is no path from Guardian to GovernorMills.acceptAdmin.

Breaking operation (the brick): GovernorMills.acceptAdmin at GovernorMills.sol:388-391

function acceptAdmin() public {
    require(msg.sender == guardian, "GovernorMills::acceptAdmin: sender must be gov guardian");
    timelock.acceptAdmin();
}

With guardian set to the Guardian contract in the constructor (GovernorMills.sol:192), and Guardian having no function that forwards to acceptAdmin, this function reverts for every possible caller:

• EOAs / other contracts: msg.sender != guardian → revert.
• Timelock executing a proposal (target=GovernorMills, sig="acceptAdmin()"): msg.sender = address(timelock) != guardian → revert.
• A re-entrant call from any oracle/token used by GovernorMills (inv, xinv): same msg.sender mismatch → revert.

Consequence — blocks the intended deployment path: Comparing against c6c9d68 (the deployed version), guardian used to be msg.sender (an EOA), which directly fixed the chicken-and-egg. The new code removes that EOA escape hatch, so for any future deployment:

1. Deploy new GovernorMills → constructor pins guardian = new Guardian(...).
2. Pass a proposal on the old governor: Timelock.setPendingAdmin(newGovernorMills).
3. Try to claim admin: only newGovernorMills.acceptAdmin() can call Timelock.acceptAdmin() (Timelock requires msg.sender == pendingAdmin). And newGovernorMills.acceptAdmin requires msg.sender == address(Guardian). No such caller exists.
4. Migration permanently stuck — Timelock.admin stays as the old governor, the new code is dead weight.

Trigger sequence (PoC scaffold):

// 1. Deploy
GovernorMills g = new GovernorMills(timelock, inv, xinv);
// g.guardian() == address(g.guardianContract())  -- the new Guardian

// 2. Old admin (whoever Timelock.admin currently is) sets pending admin
oldAdmin.execute(timelock, "setPendingAdmin(address)", abi.encode(address(g)));
// timelock.pendingAdmin() == address(g)

// 3. Try every conceivable caller
g.acceptAdmin();                     // revert: not guardian
guardianContract.deployer();         // EOA, calls Guardian — Guardian has no function for it
// → no call path exists. Stuck.

Fix (minimal): Add a single forwarder to Guardian:

function acceptGovernanceAdmin() external {
    require(msg.sender == deployer, "Guardian: not deployer");
    governorMills.acceptAdmin();
}

Alternatively, drop the guard in GovernorMills.acceptAdmin (it is permissionless-safe because Timelock.acceptAdmin itself enforces msg.sender == pendingAdmin):

function acceptAdmin() public {
    timelock.acceptAdmin();
}

---

NM-002 — GovernorMills.setGuardian() is permanently unreachable → Guardian is locked in forever

Severity: HIGH
Source: Feynman P1 (same interrogation as NM-001, different consequence)
Verification: Code trace (same enumeration as NM-001)

Coupled state: GovernorMills.guardian ↔ ability to mutate GovernorMills.guardian.
Invariant: A governance role pinned to a specific contract must be either (a) replaceable, or (b) the pinned contract must implement every privileged action — otherwise privileges become permanently inert.

Breaking operation: GovernorMills.setGuardian at GovernorMills.sol:208-213

function setGuardian(address _newGuardian) public {
    require(msg.sender == guardian, "GovernorMills::setGuardian: only guardian");
    guardian = _newGuardian;
    emit NewGuardian(guardian);
}

The only viable msg.sender is the Guardian contract, which (per NM-001's enumeration) has no function that calls setGuardian. A passing governance proposal cannot trigger it either — msg.sender during a timelock-executed action is the Timelock, not the Guardian.

Consequence — second-order amplification of NM-001:

• The current Guardian cannot be replaced.
• The Guardian is missing acceptAdmin (NM-001) and setGuardian forwarders, has 1-of-2 revert governance (NM-004), and never resets cancellableProposals (NM-007). None of those can ever be fixed — there is no upgrade path.
• Even if a future audit finds a critical bug in Guardian, the only recourse is to revert to oldGovernor (1-of-2 multisig within 180 days) and start over with a fresh GovernorMills.

Fix (minimal):

function setGuardian(address _newGuardian) public {
    require(msg.sender == guardian || msg.sender == address(timelock),
            "GovernorMills::setGuardian: only guardian or timelock");
    guardian = _newGuardian;
    emit NewGuardian(guardian);
}

Allowing the Timelock to set the guardian preserves decentralization while restoring upgradeability.

---

NM-003 — Live read of queuePeriod revives Expired proposals

Severity: MEDIUM
Source: State P2 (parallel-path comparison: votingPeriod is snapshotted at propose time, queuePeriod is not) → Feynman P3 ("WHY does state() use live queuePeriod for a historical event horizon?")
Verification: Code trace + adversarial sequence

Coupled state: Proposal.endBlock (snapshot) ↔ queuePeriod (live).
Invariant: A proposal's "Expired" verdict should be monotonic — once expired, the proposal must stay dead. Otherwise old proposals leak back into the executable set.

Code: GovernorMills.sol:426-430

} else if (proposal.eta == 0) {
    if (block.number > add256(proposal.endBlock, queuePeriod)) {
        return ProposalState.Expired;
    }
    return ProposalState.Succeeded;
}

Adversarial sequence (proposal resurrection):

1. Proposal P passes voting at block endBlock, never queued.
2. queuePeriod = 23564 blocks (~3 days). At block endBlock + 23564 + 1, state(P) → Expired.
3. Months pass. Everyone assumes P is dead.
4. A different, benign proposal Q extends queuePeriod (e.g., to 200000) — perhaps because governance wants more queueing headroom for a busy week.
5. At the very next block, state(P): block.number > endBlock + 200000? Maybe not anymore. Returns Succeeded.
6. Anyone calls queue(P) → proposal queues into Timelock.
7. After Timelock delay, execute(P) runs the old, presumed-dead actions.

Symmetric finding (quorumVotes): state() at line 424 also reads quorumVotes live. A reduction in quorum can retroactively flip a Defeated proposal to Succeeded:

• P collected 2,000 forVotes vs 0 against, quorum was 4,000 → Defeated.
• A later governance change reduces quorumVotes to 1,000.
• state(P): forVotes > againstVotes ✓, forVotes (2000) >= quorum (1000) ✓ → falls into the eta==0 branch → Succeeded.
• Anyone queues + executes P.

Pre-condition / mitigation: The governance change in step 4/5 must itself pass through full governance (Timelock execution). So the attacker needs a passing proposal that lowers quorum or extends queue period. That raises the bar — but exactly the same lever is what enables this attack, and it can be done innocently. The bug is the silent revival, not the ability to change parameters.

Fix: Snapshot queuePeriod and quorumVotes into the Proposal struct at propose() time, the same way votingDelay, votingPeriod, and xinvExchangeRates[id] already are. Use the snapshotted values in state(). This makes proposal state monotonic.

---

NM-004 — Revert is 1-of-2; Cancel is 2-of-2 (asymmetric trust on the more catastrophic action)

Severity: MEDIUM
Source: Feynman P1 Category 3 (consistency) — "Why does the more dangerous primitive have the weaker guard?"
Verification: Code trace

Code: Guardian.sol:35-46 vs 48-59

function queueRevertToPreviousGovernance(uint256 eta) external {
    require(msg.sender == deployer || msg.sender == rwg, ...);  // 1-of-2
    ...
}
function executeRevertToPreviousGovernance(uint256 eta) external {
    require(msg.sender == deployer || msg.sender == rwg, ...);  // 1-of-2
    ...
}

function allowCancel(uint256 proposalId, bool decision) external {
    require(msg.sender == deployer, ...);                       // deployer
    ...
}
function executeCancel(uint256 proposalId) external {
    require(msg.sender == rwg, ...);                            // rwg confirms — 2-of-2
    require(cancellableProposals[proposalId], ...);
    ...
}

Threat: Reverting governance hands control of the Timelock (and downstream protocol) back to oldGovernor (0x926d…9D5B). This is strictly more powerful than canceling one proposal. Yet revert requires only one of {deployer, rwg} to act, while cancelling one proposal requires both.

If either key is compromised within the 180-day window, the attacker can unilaterally queue and execute a revert. The blast radius is the entire protocol governance — far larger than a single cancellation.

Fix options:

• Symmetric (recommended): require both deployer and rwg for revert (separate approve + execute, mirroring cancel).
• Asymmetric the other way: require both for queue, allow either for execute. This way a compromise of one key cannot start a revert, but cannot block one that the other party initiated.

---

NM-005 — executeRevertToPreviousGovernance lacks the revertDeadline check

Severity: LOW
Source: Feynman P1 (Category 3 consistency) — "Why does queue check the deadline but execute doesn't?"
Verification: Code trace

Code: Guardian.sol:35-46

function queueRevertToPreviousGovernance(uint256 eta) external {
    require(msg.sender == deployer || msg.sender == rwg, ...);
    require(block.timestamp <= revertDeadline, "Guardian: revert deadline exceeded");  // ← checked
    ...
}
function executeRevertToPreviousGovernance(uint256 eta) external {
    require(msg.sender == deployer || msg.sender == rwg, ...);
    // ← deadline NOT checked
    ...
}

Effect: The "6-month rollback window" can be effectively extended by timelock.delay + GRACE_PERIOD (up to 30 days + 14 days = 44 days). A revert queued on day 179 with eta = day 179 + delay can be executed up to (eta + 14d) after the official deadline.

Severity rationale: Bounded by Timelock's GRACE_PERIOD. The intended semantics ("revert is possible during a 6-month window") are violated only in a narrow extension; a determined deployer/rwg could exploit the slack to act after the deadline they publicly committed to.

Fix:

function executeRevertToPreviousGovernance(uint256 eta) external {
    require(msg.sender == deployer || msg.sender == rwg, "...");
    require(block.timestamp <= revertDeadline, "Guardian: revert deadline exceeded");
    ...
}

This forces both queue and execute to fall inside the deadline.

---

NM-006 — cancel() measures CURRENT xinv balance against an OLD exchange rate

Severity: LOW
Source: Feynman P1 (Category 4: assumptions about state freshness) → State P2 confirmed mixed reference frame
Verification: Code trace + worked numeric example

Coupled mismatch: proposer's xinv balance at block.number-1 is current; xinvExchangeRates[id] is captured at propose time. The result is an INV-equivalent voting-power figure that corresponds to no actual point in time.

Code: GovernorMills.sol:302

require(msg.sender == guardian || (
    _getPriorVotes(proposal.proposer, sub256(block.number, 1), xinvExchangeRates[proposal.id])
        < proposalThreshold && !proposerWhitelist[proposal.proposer]
), "GovernorMills::cancel: proposer above threshold");

Numeric exploit example (rate rises, which is the common case for an interest-accruing cToken):

• proposalThreshold = 1000 INV
• At T0: proposer holds 0 INV + 800 xINV; xinv exchangeRate = 1.5 → 1200 effective INV. They propose. xinvExchangeRates[id] = 1.5.
• Later, proposalThreshold is raised to 2000 (via a passing proposal). Exchange rate has risen to 5.0 (it is a multi-month proposal lifecycle).
• Real voting power: 800 × 5.0 = 4000 INV — above threshold. Proposer should NOT be cancellable.
• cancel() evaluates: 800 × 1.5 = 1200 < 2000 → non-guardian cancel succeeds.

The proposer is canceled despite legitimately holding enough voting power, because the stale rate undercounts their xinv contribution.

Symmetric case (rate falls — possible if xinv has any loss-realization mechanism): stale rate overcounts voting power and prevents legitimate cancellation of a proposer whose real power has dropped.

Fix: Use xinv.exchangeRateStored() (or exchangeRateCurrent if the side-effect of accrueInterest is acceptable in cancel()) to compute against the current rate. Alternatively, mirror voting and use both the snapshotted balance (proposal.startBlock) AND snapshotted rate — but this prevents canceling a proposer who has since dumped their xinv, which may not be the desired tradeoff.

---

NM-007 — cancellableProposals[id] is never reset after executeCancel; allows pre-approval of future proposals

Severity: LOW
Source: State P2 (mutation matrix — only one writer to cancellableProposals, no clearing path) → Feynman P3 ("WHY would allowCancel be permitted for a not-yet-existing id?")
Verification: Code trace

Code: Guardian.sol:48-59

function allowCancel(uint256 proposalId, bool decision) external {
    require(msg.sender == deployer, ...);
    cancellableProposals[proposalId] = decision;   // no bound check on proposalId
    emit AllowCancel(proposalId, decision);
}

function executeCancel(uint256 proposalId) external {
    require(msg.sender == rwg, ...);
    require(cancellableProposals[proposalId], ...);
    governorMills.cancel(proposalId);
    // ← no reset of cancellableProposals[proposalId]
    emit ExecuteCancel(proposalId);
}

Two issues:

1. Pre-approval of unborn proposals. Proposal IDs are monotonically increasing and predictable (proposalCount + 1, +2, ...). A deployer can pre-mark, say, id proposalCount + 5 as cancellable. Five proposals later, that slot is filled by whatever proposer happens to land there, and rwg can cancel it. A future compromise of the deployer key combined with a current rwg signature is sufficient.
2. No reset after use. After a successful cancel, the flag remains true. Re-calling executeCancel(id) is gas-wasteful but does no further harm because governorMills.cancel(id) is idempotent on already-canceled proposals. Mostly a hygiene issue.

Fix:

function allowCancel(uint256 proposalId, bool decision) external {
    require(msg.sender == deployer, ...);
    require(proposalId <= governorMills.proposalCount(), "Guardian: id not yet exists");  // requires exposing proposalCount via interface
    cancellableProposals[proposalId] = decision;
    emit AllowCancel(proposalId, decision);
}

function executeCancel(uint256 proposalId) external {
    require(msg.sender == rwg, ...);
    require(cancellableProposals[proposalId], ...);
    cancellableProposals[proposalId] = false;   // reset
    governorMills.cancel(proposalId);
    emit ExecuteCancel(proposalId);
}

---

NM-008 — Silent uint96 truncation in _getPriorVotes

Severity: INFO (not exploitable at current INV/xINV supply)
Source: Feynman P1 (Category 5: boundary)
Verification: Magnitude analysis

Code: GovernorMills.sol:197-203

uint96 xinvPriorVotes = uint96(
    (
        uint256(xinv.getPriorVotes(_proposer, _blockNumber)) * _exchangeRate
    ) / 1 ether
);

uint256 multiplication is checked, the divide is safe, but the final uint96 cast silently truncates if the product/1e18 exceeds 2^96 ≈ 7.9 × 10^28. With INV total supply on the order of 10^23 and rate ~10^18, the realistic per-voter result is well within 2^96. Not exploitable today, but should use require(value <= type(uint96).max) to harden against future supply / rate growth.

---

NM-009 — cancel() permits no-op re-cancellation; no early-return on already-canceled proposal

Severity: INFO
Source: Feynman P1 (Category 5: double-call boundary)
Verification: Code trace

Code: GovernorMills.sol:297-310. After a proposal is canceled, calling cancel(id) again:

• state == Canceled, not Executed → passes the first require.
• Threshold check evaluates against current proposer state (may pass).
• proposal.canceled = true is a no-op.
• Loop re-calls timelock.cancelTransaction(...) for each action — also a no-op on already-canceled tx hashes.

No security impact; gas waste only. An early-return on state == Canceled would tidy this up.

---

NM-010 — updateProposerWhitelist lacks the "no change in value" check used by sibling update functions

Severity: INFO
Source: Feynman P1 (Category 3: consistency)
Verification: Code trace

updateProposalThreshold, updateProposalQuorum, updateVotingDelay, updateVotingPeriod, updateQueuePeriod all reject identical-value updates. updateProposerWhitelist does not. Minor inconsistency; emits redundant ProposerWhitelistUpdated events on idempotent calls.

---

Feedback-Loop Discoveries (highest-value findings)

Two findings emerged only because Feynman and State cross-fed each other:

1. NM-001 + NM-002: Feynman noticed the guard was unusual (msg.sender == guardian with guardian being a contract). State Mapper then enumerated outbound calls from that contract and proved no call path exists. Neither auditor alone would frame this as a bug — Feynman would mark it "SUSPECT, possibly intentional"; State would map the addresses without questioning why the contract has no reciprocal function. The combination produces the "permanently unreachable" verdict.

2. NM-003 (live-read of queuePeriod): State Mapper flagged queuePeriod as a coupled-but-unsnapshotted value (parallel path: votingPeriod IS snapshotted). Feynman P3 then asked "WHY is one snapshotted and not the other?" — exposing the intent gap and the resurrection sequence.

---

False Positives Eliminated During Verification

• Reentrancy in execute(): proposal.executed = true is set before the external-call loop, and state() returns Executed on re-entry, blocking nested execute(P). Re-entry into other functions (cancel/queue/propose for different IDs) is permitted but offers no exploit path that the same caller couldn't take directly.
• Signature replay in castVoteBySig: Domain separator includes chainId and address(this); per-voter Receipt.hasVoted prevents same-chain replay. No issue.
• Voting period changes affecting in-flight proposals: endBlock is snapshotted at propose() time. Subsequent updateVotingPeriod calls do not affect existing proposals. Distinct from NM-003 (which is about queuePeriod only).
• Race between execute() and cancel(): Both atomically transition state; once one wins, the other reverts. No exploit.

---

Summary

• Total functions analyzed: 30 (GovernorMills: 23, Guardian: 7, Timelock read-only for context)
• Coupled state pairs mapped: 13
• Nemesis loop iterations: 4 (converged after Pass 4 yielded no new findings)
• Feedback-loop discoveries: 2 (NM-001/002 cluster, NM-003)
• After verification: 10 TRUE POSITIVE, 0 false positive, 0 downgraded
• Severity breakdown: 2 HIGH · 2 MEDIUM · 3 LOW · 3 INFO

Most urgent items:

• NM-001 / NM-002 must be fixed before the new GovernorMills (with integrated Guardian) is deployed as a Timelock admin. As-is, the migration cannot complete and the Guardian cannot ever be replaced. The fixes are tiny (add one Guardian forwarder, or relax the acceptAdmin guard).
• NM-003 is the most exploit-shaped of the remaining findings — snapshot queuePeriod and quorumVotes into Proposal to make state monotonic.

[08xmt]

NM-001 - Left in acceptAdmin so as to touch as little code as possible. Can be removed.
NM-002 - Left in setGuardian() so as to touch as little code as possible. Can be removed.
NM-003 - Fine as queuePeriod is unlikely to be set to very long, and queuePeriod exists specifically to deal with long range attacks
NM-004 - Reverting may be more time sensitive and is a time limited function. Can be turned into a 2-of-2 if deemed necessary.
NM-005 - Not really an issue, as must be executed within grace period of timelock, but can add stricter requirement of being executed within revertDeadline.
NM-006 - Fine as proposals are short lived, we have whitelisted proposers and xINV gains value very slowly.
NM-007 - Fixed in bd68d95
NM-008 - Extremely unlikely to ever be an issue. INV supply would have to increase dramatically. Worst case scenario is that large holder will have to split INV over multiple wallets.
NM-009 - Not an issue.
NM-010 - Not an issue.

[nourharidy]

NM-01: How else is GovernorMills expected to accept ownership of the Timelock? I don't think it can be done by governance vote because executing a vote requires ownership of the Timelock already. Please add integration fork tests to verify the whole flow.

NM-02: LGTM.
NM-03: LGTM.
NM-04: LGTM current setup is ok
NM-05: LGTM. Not an issue.
NM-06: LGTM. It's an old, known issue
NM-07: LGTM.
NM-08: LGTM.