security: registerDINaggregator has no onlyCurrentGI — validators can pre-register for (and fill) a future GI's aggregator list

[umeradl]

Summary

Found while reviewing PR No. 204 (verification comment, surfaced item B). This is older than PR No. 204 and out of its scope. Line numbers are for develop @ e373c8d.

DINTaskCoordinator.registerDINaggregator(_GI) doesn't check that _GI is the current GI. While any GI's aggregator registration window is open, a validator can register for a future GI, which fills that GI's aggregator list before its own window opens.

Problem

foundry/src/DINTaskCoordinator.sol:397:

function registerDINaggregator(uint _GI) public {
    if (GIstate != GIstates.DINaggregatorsRegistrationStarted) revert ...;
    ...
    if (isDINAggregator[_GI][msg.sender]) revert TC_AggregatorAlreadyRegistered();
    if (dinAggregators[_GI].length >= MAX_REGISTERED_AGGREGATORS) revert TC_RegistrationCapReached();
    ...
    dinAggregators[_GI].push(msg.sender);
    isDINAggregator[_GI][msg.sender] = true;
    dinvalidatorStakeContract.incrementActiveRegistration(msg.sender);

The state check is on the current GI, but every read and write uses the caller's _GI. The auditor side does check: DINTaskAuditor.registerDINAuditor(uint _GI) public onlyCurrentGI(_GI) (DINTaskAuditor.sol:660), and the coordinator has the same modifier (L246). dincli always passes curr_GI (dincli/cli/aggregator.py:139), so honest users are unaffected.

Failure scenario

Proven with a throwaway forge test (run, deleted, not committed): during GI 1's registration window, registerDINaggregator(2) succeeds. isDINAggregator(2, v) becomes true, isDINAggregator(1, v) stays false, and activeRegistrationCount(v) == 1.

Consequences:

1. Capturing a future GI's aggregation. During GI N's window, an attacker registers 300 addresses (MAX_REGISTERED_AGGREGATORS) for GI N+1. When GI N+1's window opens, every honest validator hits TC_RegistrationCapReached, and autoCreateTier1AndTier2 builds all T1/T2 batches from the attacker's addresses (those still Active). They pick every batch's CID. The only defense is an owner-adjudicated S4 dispute. The cost is 300 × MIN_STAKE (10 DIN by default, about 0.003 ETH at the default faucet rate), minus any per-model floor or concurrency cap if configured. In normal registration, honest validators at least compete inside the window; pre-registration removes that.
2. Bypassing the owner's timing. Registrations land for a GI whose window the owner never opened.
3. Leaked registration slots. Registering for an already-ended GI whose slots were released increments activeRegistrationCount. releaseGIRegistrationSlots (L1261) won't run again for that GI, so the slot is never freed. This only affects the caller.

Fix

Add onlyCurrentGI(_GI) to registerDINaggregator, matching registerDINAuditor, plus a regression test: registerDINaggregator(GI + 1) and registerDINaggregator(GI - 1) revert with TC_WrongGI.

Size note: DINTaskCoordinator is already 9 B over EIP-170 on develop (No. 201 Part A), so this change must land together with, or after, No. 201's size reduction. Check forge build --sizes in the PR.

Related

No. 201 (contract size), No. 180 (dual-role registration), PR No. 204.

[umeradl]

Done by PR No. 211, merged into develop as 84d0eef, with a docs-only review fix in 420d48d. The develop push CI run is green, and there's an outcome comment on the PR. Closing by hand, because a merge into develop doesn't trigger closing keywords (main is the default branch).

Checked on origin/develop @ 420d48d:

• Fix: registerDINaggregator(uint _GI) public onlyCurrentGI(_GI) (foundry/src/DINTaskCoordinator.sol:401) matches DINTaskAuditor.registerDINAuditor. It shipped as its own commit, c3e585d, so it can be reviewed and reverted on its own (task_021026_19 Part B).
• Regression tests (foundry/test/StakingEnforcement.t.sol:296-324):
  • test_registerDINaggregator_futureGI_reverts (GI + 1) and _pastGI_reverts (GI − 1) both expectRevert(TC_WrongGI) and assert activeRegistrationCount stays 0, so no slot leaks.
  • _currentGI_registers covers the honest path.
  • In review, removing the modifier made exactly the future/past tests fail.
• Size note: this landed together with issue No. 201 Part A in the same PR. It costs +9 B, and DINTaskCoordinator is 23,497 B runtime (1,079 B margin). The new CI size gate passes on develop.
• Docs: DINTaskCoordinator.md §6.2 now states onlyCurrentGI, and §10 No. 11 is marked fixed.
• Verification: forge test 481 passed, 0 failed, on the merged tree. The develop push run is green, including the contract size gate step.

Related: issue No. 201 stays open for Part B (the slash reason for committed-but-unrevealed).