fix: bind Incentives to the vault's live wiring in SystemSealer - #15
Conversation
INVARIANT 8 checked Incentives.owner() == ROOT_TIMELOCK but never that config.incentives was the address CoreVault actually reads from — the same decoy gap already closed for feeCollector/router/bufferManager/ healthRegistry, just missed for this component. A correctly-governed Incentives contract the vault doesn't point to could still pass canSeal()/verifyAndSeal(). _verifyLiveState now checks vault.incentives() == config.incentives before the ownership check, matching the existing pattern. Added test_canSeal_and_verifyAndSeal_agree_whenIncentivesIsADecoy, wiring a real Incentives contract into the vault and proving both canSeal() and verifyAndSeal() reject a separate, correctly-owned decoy.
stefanobotticelli
left a comment
There was a problem hiding this comment.
Reviewed at a8414b0. Correct and mergeable: 946 tests, 101 suites, green, exactly +1 over baseline with no assertions removed. configHash and both selectors are byte-identical to base, so no deploy tooling to regenerate.
Verified vault.incentives() is the same slot the production path reads — ERC4626Module._notifyIncentivesDeposit (:849) reads core.incentives, single writer AdminModule.setIncentives. Not an alias, so this isn't a repeat of the GlobalConfig case.
Verified the test by mutation: remove the three binding lines and test_canSeal_and_verifyAndSeal_agree_whenIncentivesIsADecoy fails with canSeal returning (true, "") — the decoy passes every other invariant including ownership, since it's constructed with owner = rootTimelock. The test discriminates exactly the binding.
Doesn't block 1 September: shadow runs with DEPLOY_INCENTIVES=false and config.incentives = address(0), and I confirmed that branch seals cleanly.
One thing worth folding in while you're here. The check sits inside if (config.incentives != address(0)), whereas INVARIANT 3/5/6/7 are unconditional. That leaves one case open:
config.incentives |
vault.incentives() |
Result |
|---|---|---|
| 0 | 0 | seals — correct |
| 0 | live module, hostile owner | seals |
| decoy | real | rejected — correct |
An Incentives contract genuinely wired into the vault but declared as zero in the manifest escapes both the bind and the ownership check, and gets sealed in permanently. Pre-existing — the ownership check was already behind the same gate — but this is the PR that closes that gap.
The precedent is your own INVARIANT 8e for recoveryGate: equality check outside the gate, then enter the branch for the identity checks. One line.
Two smaller ones: the header checklist at SystemSealer.sol:74 still reads only "Incentives.owner == ROOT_TIMELOCK (if deployed)" without mentioning the bind, unlike the feeCollector/router/bufferManager/healthRegistry entries. And separately from this PR, DeployCoreSystem never wires incentives at all — setEcosystem hardcodes address(0) at :469, setIncentives appears nowhere in script/, and when DEPLOY_INCENTIVES=true the module is deployed with owner = cfg.deployer and never handed to the root timelock. Not a problem now, but it has to be fixed before incentives are ever actually enabled.
INVARIANT 8 checked Incentives.owner() == ROOT_TIMELOCK but never that config.incentives was the address CoreVault actually reads from — the same decoy gap already closed for feeCollector/router/bufferManager/ healthRegistry, just missed for this component. A correctly-governed Incentives contract the vault doesn't point to could still pass canSeal()/verifyAndSeal().
_verifyLiveState now checks vault.incentives() == config.incentives before the ownership check, matching the existing pattern. Added test_canSeal_and_verifyAndSeal_agree_whenIncentivesIsADecoy, wiring a real Incentives contract into the vault and proving both canSeal() and verifyAndSeal() reject a separate, correctly-owned decoy.