diff --git a/Developer/design/adversarial-threat-model.md b/Developer/design/adversarial-threat-model.md index 553ae02f..1b441003 100644 --- a/Developer/design/adversarial-threat-model.md +++ b/Developer/design/adversarial-threat-model.md @@ -80,9 +80,9 @@ entry in `foundry/test/P3Adversarial.t.sol`. |---|---| | **Attacker** | One address registered as both auditor and aggregator in the same GI; model owner tries to steer the batch-assignment draw (formerly by choosing which block to submit batch-creation calls in) | | **Capability** | One address (auditor + aggregator), staking MIN_STAKE. Before issue #156 H-2 (PR #191) the owner chose which block `autoCreateTier1AndTier2` and `createAuditorsBatches` were submitted in, picking a `blockhash(block.number - 1)` that routed the poisoned model into the colluding T1 batch. Both now read a locked, future-block-anchored `aggSeed`/`auditSeed` (see Row 2), so that lever is closed; what remains is the BL-26 re-roll (decline to lock, wait out the ~256-block window) and TA-4 | -| **Protocol response today** | Nothing prevents dual-role registration: `registerDINaggregator` (`DINTaskCoordinator.sol:386`) and `registerDINAuditor` (`DINTaskAuditor.sol:660`) have no cross-role check. Both increment `activeRegistrationCount`, so the concurrent-registration cap only raises the minimum stake required (≥ 2 concurrent registrations' worth) — it does not block. The cap is also off by default (`maxConcurrentRegistrationsPerStakeUnit == 0`, `DINTaskCoordinator.sol:406`). Note: a per-address cross-role guard is Sybil-bypassable (second address with its own stake), so the stake cost may be the only practical defence in any case. | -| **Expected test outcome** | **KNOWN GAP** ([#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180); the [#156](https://github.com/InfiniteZeroFoundation/DevNet/issues/156) block-selection half is fixed, residuals in BL-26) — the test registers one address in both roles for the same GI and asserts it succeeds. The "colluding aggregators skip outlier clipping" step is off-chain (model-dependent, not testable in Forge). | -| **Trust assumption** | None that defends this — gap pending #180 (role-separation or stake-cost documentation) and #156 (stronger batch-shuffle seed). | +| **Protocol response today** | One address can no longer hold both roles in a GI: `registerDINAuditor` (`DINTaskAuditor.sol:674`) reverts with `TA_DualRoleNotAllowed` when the caller is already an aggregator for that GI, read from the coordinator's `isDINAggregator` (#180). The check sits on the auditor side because aggregator registration (`registerDINaggregator`, `DINTaskCoordinator.sol:401`; states 6–7) always comes before auditor registration (8–9). It is per address: a second address with its own stake still gets both roles (Sybil), so the remaining defence is that the attack costs a second stake and is visible on-chain. The concurrent-registration cap (off by default, `maxConcurrentRegistrationsPerStakeUnit == 0`) can raise that cost further when it is non-zero. | +| **Expected test outcome** | **DEFENDED** for the single-address case ([#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180), `StakingEnforcement.t.sol` `test_registerDINAuditor_aggregatorSameGI_reverts`); the Sybil variant is documented by `test_registerDINAuditor_secondAddressWithOwnStake_registers`. The [#156](https://github.com/InfiniteZeroFoundation/DevNet/issues/156) block-selection half is fixed, residuals in BL-26. The "colluding aggregators skip outlier clipping" step is off-chain (model-dependent, not testable in Forge). PR #218's `test_knownGap_dualRoleRegistration` flips to `test_defended_…` when it rebases on this change. | +| **Trust assumption** | Operators don't split one stake-holder across two funded addresses to take both roles (Sybil); the cost is a second stake. Batch-shuffle residuals as in BL-26. | --- @@ -164,7 +164,7 @@ entry in `foundry/test/P3Adversarial.t.sol`. |---|---|---|---| | S3 graduating from shadow mode | [#38](https://github.com/InfiniteZeroFoundation/DevNet/issues/38) | Row 4 (auditor bloc poisoned model) | `s3SlashingEnabled` set to `true` after threshold calibration | | T1 wrong CID — finalCID not replaced, dissenter not refunded | [#194](https://github.com/InfiniteZeroFoundation/DevNet/issues/194) (#181 covers who adjudicates, not repairing the outcome) | Row 5 (T1 wrong CID) | `settleRecomputation(confirmed=true)` replaces `finalCID` and refunds the honest dissenter | -| Dual-role registration guard | [#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180) | Row 6 (cross-role + owner block-selection) | Per-address role-separation guard added, or the stake cost documented as the intended defence (with a non-zero concurrent-registration cap) | +| Dual-role registration guard | [#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180) | Row 6 (cross-role + owner block-selection) | **Flipped by task_061026_20 Part B:** per-address guard in `registerDINAuditor` (`TA_DualRoleNotAllowed`). The Sybil variant (second funded address) remains, bounded by stake cost | | Stronger batch-shuffle seed | [#156](https://github.com/InfiniteZeroFoundation/DevNet/issues/156) | Rows 2, 6 (owner block-selection) | **Flipped by PR #191:** shuffles seeded by the locked, future-block `aggSeed`/`auditSeed` rather than at owner-controlled call time. Residuals (decline-to-lock re-roll, post-lock pool reshaping) tracked in BL-26 | | Decentralized dispute adjudication | [#181](https://github.com/InfiniteZeroFoundation/DevNet/issues/181) | Row 7 (owner suppresses dispute) and bond-locked-forever variant | `resolveDispute` gated by multi-sig, DAO committee, or fraud-proof; timeout added for unresolved disputes | | Dispute timeout for unresolved disputes | [#181](https://github.com/InfiniteZeroFoundation/DevNet/issues/181) | Row 7 (owner never resolves → bond locked) | `expireDispute` (or a new function) reachable when `d.resolved == false` after a deadline | @@ -179,5 +179,5 @@ entry in `foundry/test/P3Adversarial.t.sol`. 1. **Row 5 — T1 wrong CID: KNOWN GAP.** `settleRecomputation(confirmed=true)` does not replace `finalCID` and does not refund the honest dissenter's `AGG_T1_BAD_CONSENSUS` slash, so the attack partially succeeds regardless of TA-1. COST-BOUNDED was rejected because it would pass a test while the honest dissenter is never made whole. Test: `test_knownGap_t1WrongCID`. -2. **Row 6 — Dual-role registration: KNOWN GAP.** - Not accepted as intended; [#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180) stays open. A per-address cross-role guard is Sybil-bypassable (a second address with its own stake), so #180 decides between adding the guard and documenting the stake cost as the defence. The concurrent-registration cap only bounds that cost when it is non-zero. +2. **Row 6 — Dual-role registration: DEFENDED (single address).** + Not accepted as intended. [#180](https://github.com/InfiniteZeroFoundation/DevNet/issues/180) chose the per-address guard (PR #219 Decision 2): `registerDINAuditor` refuses an address already registered as an aggregator for the GI. It is Sybil-bypassable (a second address with its own stake), which the tests document; the concurrent-registration cap bounds that cost further only when it is non-zero. diff --git a/Documentation/technical/contracts/DINShared.md b/Documentation/technical/contracts/DINShared.md index 659d465e..474712c8 100644 --- a/Documentation/technical/contracts/DINShared.md +++ b/Documentation/technical/contracts/DINShared.md @@ -158,6 +158,7 @@ Used by: `DINTaskCoordinator`, `DINTaskAuditor` interface IDINTaskCoordinator { function GI() external view returns (uint256); function GIstate() external view returns (GIstates); + function isDINAggregator(uint256 gi, address aggregator) external view returns (bool); function aggregatorWeight(uint256 gi, address aggregator) external view returns (uint256); } ``` @@ -168,6 +169,7 @@ Used by: `DINTaskAuditor` |--------|---------| | `GI()` | Current GI counter, for `onlyCurrentGI` and `depositRewards` validation | | `GIstate()` | Lifecycle gate for every auditor-side phase | +| `isDINAggregator` | Read by `registerDINAuditor` to refuse an address already registered as an aggregator for the GI (issue No. 180) | | `aggregatorWeight` | Read at `claimReward` time to compute each aggregator's weighted share | ### 3.3 `IDINTaskAuditor` @@ -227,6 +229,7 @@ Used by: `DINTaskCoordinator` | `TA_AuditorRegistrationNotOpen` | Registration attempted outside registration window | | `TA_WrongGI` | Global Iteration mismatch | | `TA_AuditorAlreadyRegistered` | Duplicate auditor registration for same GI | +| `TA_DualRoleNotAllowed` | `registerDINAuditor` by an address already registered as an aggregator for the same GI (issue No. 180) | | `TA_LMSubmissionsNotOpen` | Local model submission attempted outside submission window | | `TA_AlreadySubmitted` | Client has already submitted a model this GI | | `TA_MaxLMSubmissionsReached` | Submission count reached `MAX_LM_SUBMISSIONS` (10,000) | diff --git a/Documentation/technical/contracts/DINTaskAuditor.md b/Documentation/technical/contracts/DINTaskAuditor.md index 740bf81d..5b667def 100644 --- a/Documentation/technical/contracts/DINTaskAuditor.md +++ b/Documentation/technical/contracts/DINTaskAuditor.md @@ -77,12 +77,14 @@ The reward split and the S1 fraction are explicitly provisional (MECHANISM_DESIG ### 3.4 Per-GI data -- **Registration:** `dinAuditors[gi]`, `isRegisteredAuditor[gi][addr]`. +Mappings marked *(internal)* have no auto-getter; read them through the view named next to them (issue No. 201 Part A item 4). + +- **Registration:** `dinAuditors[gi]` *(internal; read with `getDINtaskAuditors(gi)`)*, `isRegisteredAuditor[gi][addr]`. - **Submissions:** `lmSubmissions[gi]` (`LMSubmission`: client, modelCID, submittedAt, eligible, evaluated, approved, finalMedianScore), `clientHasSubmitted`, `clientSubmissionIndex`. -- **Batches:** `auditBatches[gi]` (`AuditBatch`: batchId, auditors, modelIndexes, testDataCID), `isBatchAuditor`, `isBatchModelIndex`. +- **Batches:** `auditBatches[gi]` *(internal; read with `getAuditorsBatch(gi, batchId)`)* (`AuditBatch`: batchId, auditors, modelIndexes, testDataCID), `isBatchAuditor`, `isBatchModelIndex`. - **Scoring:** `auditScoreCommits`, `hasCommittedLM`, `auditScores`, `LMeligibleVote`, `hasAuditedLM` (all keyed `[gi][batchId][auditor][modelIndex]`). -- **Test data:** `encryptedTestDataKey[gi][batchId][auditor]`, `testDataCommitments[gi][batchId]`, `Is_testdataCIDs_Assigned[gi]`, `testDataDisputes[gi][batchId]`. -- **Rewards:** `giRewardPool[gi]`, `giRewardSnapshot[gi]`, `giTotalApprovedScore[gi]`, `giTotalAuditWeight[gi]`, `auditorGIWeight[gi][addr]`, `rewardClaimed[gi][addr]`, `claimable[addr]`, `treasuryAccrued`. +- **Test data:** `encryptedTestDataKey[gi][batchId][auditor]`, `testDataCommitments[gi][batchId]`, `Is_testdataCIDs_Assigned[gi]` *(internal; only guards `setTestDataAssignedFlag` against a second call)*, `testDataDisputes[gi][batchId]`. +- **Rewards:** `giRewardPool[gi]`, `giRewardSnapshot[gi]`, `giTotalApprovedScore[gi]`, `giTotalAuditWeight[gi]`, `auditorGIWeight[gi][addr]` *(internal; feeds `claimReward`)*, `rewardClaimed[gi][addr]`, `claimable[addr]`, `treasuryAccrued`. --- @@ -101,7 +103,7 @@ The reward split and the S1 fraction are explicitly provisional (MECHANISM_DESIG ## 5. Registration & Submissions -- **`registerDINAuditor(gi)`** — requires the coordinator state `DINauditorsRegistrationStarted`. Checks: not already registered, fewer than 300 registered (`TA_RegistrationCapReached`), `isValidatorActive` (`TA_AuditorNotActive`), stake ≥ the model floor (`TA_StakeBelowModelFloor`), and the concurrent-registration cap (`TA_ConcurrentRegistrationCapReached`). Then calls `incrementActiveRegistration` and emits `DINAuditorRegistered`. +- **`registerDINAuditor(gi)`** — requires the coordinator state `DINauditorsRegistrationStarted`. Checks: not already registered, not already an aggregator for the same GI (`TA_DualRoleNotAllowed`, read from the coordinator's `isDINAggregator`; issue No. 180), fewer than 300 registered (`TA_RegistrationCapReached`), `isValidatorActive` (`TA_AuditorNotActive`), stake ≥ the model floor (`TA_StakeBelowModelFloor`), and the concurrent-registration cap (`TA_ConcurrentRegistrationCapReached`). Then calls `incrementActiveRegistration` and emits `DINAuditorRegistered`. - **`submitLocalModel(cid, gi)`** — state `LMSstarted`; one submission per address per GI; up to 10 000 per GI. No stake or registration needed. --- @@ -221,3 +223,5 @@ Read alongside the [foundry/src security review](../audits/foundry-src-security- - `createAuditorsBatches` takes the coordinator's locked audit seed (issue No. 156 H-2, PR No. 191). - The audit commit hash binds the auditor and the slot: `keccak256(abi.encode(score, vote, salt, msg.sender, gi, batchId, modelIndex))` replaces `keccak256(abi.encodePacked(score, vote, salt))` (issue No. 192). Function signatures and the ABI are unchanged; in-flight commits made under the old formula can't be revealed after the switch. - Test-data disputes (issue No. 205): only an auditor of the batch can open one; `resolveTestDataDispute` is owner-only; `closeExpiredDispute` now upholds an unanswered dispute instead of forfeiting the bond; `disputeBondAmount` defaults to 100 DIN. `DisputeExpired` drops its `bondForfeited` field. Disputes can't be opened once the GI is settled (`TA_RewardsAlreadySettled`), and one upheld after settlement takes no pool penalty, so settled claims stay funded (PR No. 215 review). +- Size review (issue No. 201 Part A item 4): `auditBatches`, `dinAuditors`, `Is_testdataCIDs_Assigned` and `auditorGIWeight` lose their auto-getters (nothing outside the contract read them; `getAuditorsBatch` / `getDINtaskAuditors` cover the first two). `assignAuditTestDataset` and `reassignAuditTestDataset` share `_storeTestData` and `_batchAt`, and `_burnAndForward` forwards its half through `_forwardToTreasury`. Behaviour and events are unchanged; runtime size drops from 22,718 B to 21,188 B. +- Dual-role guard (issue No. 180): `registerDINAuditor` reverts with `TA_DualRoleNotAllowed` when the caller is already an aggregator for the same GI. The check is on the auditor side because aggregators always register first; it is per address, so a second address with its own stake can still take the auditor role. `IDINTaskCoordinator` gains `isDINAggregator`. diff --git a/Documentation/technical/contracts/DinValidatorStake.md b/Documentation/technical/contracts/DinValidatorStake.md index 570d82c7..71028deb 100644 --- a/Documentation/technical/contracts/DinValidatorStake.md +++ b/Documentation/technical/contracts/DinValidatorStake.md @@ -309,7 +309,7 @@ Until step 8, slasher management through the coordinator reverts `ValidatorStake - **No. 1 — S6 is not wired:** `recordNoParticipation` exists and is tested, but no task contract calls it, so the S6 counter never moves in practice. - **No. 2 — S5 escalation on a blacklisted validator reverts the whole slash:** escalation calls `_jailInternal`, which reverts `ValidatorIsBlacklisted`. A task contract's slashing loop hitting a blacklisted repeat offender would revert, not just skip that validator. - **No. 3 — Jail exit does not require `reactivate()`:** after `jailedUntil` passes, any status-syncing call (e.g. `stake`) recomputes the status, bypassing `reactivate()`'s `StakeBelowFloor` check (the later sync still requires `≥ MIN_STAKE` for `Active`). -- **No. 4 — Stale NatSpec:** `setModelStakeBounds` / `setMaxConcurrentRegistrationsPerStakeUnit` say "not yet enforced", but the task contracts enforce both (§8). `getModelStakeMin` says "set by the model owner", but the setter is `onlyOwner` (DIN-Representative). `modelMinStakeBounds[].max` is never read. +- **No. 4 — Stale NatSpec (partly fixed, issue No. 180):** the `setModelStakeBounds` / `setMaxConcurrentRegistrationsPerStakeUnit` NatSpec now says where the task contracts enforce both (§8), `activeRegistrationCount` names `releaseGIRegistrationSlots` as the decrement, and `getModelStakeMin` names the `onlyOwner` (DIN-Representative) setter instead of "the model owner". Still open: `modelMinStakeBounds[].max` is never read. - **No. 5 — Recidivism is per model:** the S5 ring is keyed by calling contract, so a validator faulting across many models never escalates unless it hits the threshold within one model. - **No. 6 — Blacklisted funds are frozen:** blacklisted validators cannot unstake or claim; there is no recovery path other than unblacklisting. - **No. 7 — Custody meets upgradeability:** see §13. diff --git a/dincli/abis/DINTaskAuditor.json b/dincli/abis/DINTaskAuditor.json index fd5512bc..2a6bb6c9 100644 --- a/dincli/abis/DINTaskAuditor.json +++ b/dincli/abis/DINTaskAuditor.json @@ -40,25 +40,6 @@ ], "stateMutability": "view" }, - { - "type": "function", - "name": "Is_testdataCIDs_Assigned", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "outputs": [ - { - "name": "", - "type": "bool", - "internalType": "bool" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "LMeligibleVote", @@ -158,35 +139,6 @@ "outputs": [], "stateMutability": "nonpayable" }, - { - "type": "function", - "name": "auditBatches", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "outputs": [ - { - "name": "batchId", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "testDataCID", - "type": "bytes", - "internalType": "bytes" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "auditScoreCommits", @@ -255,30 +207,6 @@ ], "stateMutability": "view" }, - { - "type": "function", - "name": "auditorGIWeight", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "outputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "claimReward", @@ -467,30 +395,6 @@ "outputs": [], "stateMutability": "nonpayable" }, - { - "type": "function", - "name": "dinAuditors", - "inputs": [ - { - "name": "", - "type": "uint256", - "internalType": "uint256" - }, - { - "name": "", - "type": "uint256", - "internalType": "uint256" - } - ], - "outputs": [ - { - "name": "", - "type": "address", - "internalType": "address" - } - ], - "stateMutability": "view" - }, { "type": "function", "name": "dinToken", @@ -2559,6 +2463,11 @@ "name": "TA_DisputeWindowClosed", "inputs": [] }, + { + "type": "error", + "name": "TA_DualRoleNotAllowed", + "inputs": [] + }, { "type": "error", "name": "TA_EmptyCommitHash", diff --git a/foundry/src/DINShared.sol b/foundry/src/DINShared.sol index b325fe08..a27f1e81 100644 --- a/foundry/src/DINShared.sol +++ b/foundry/src/DINShared.sol @@ -112,6 +112,12 @@ interface IDINTaskCoordinator { function GIstate() external view returns (GIstates); + /// @dev Whether `aggregator` registered as an aggregator for GI `gi` + /// (the coordinator's public mapping). Read by + /// DINTaskAuditor.registerDINAuditor to refuse dual-role registration + /// (issue No. 180). + function isDINAggregator(uint256 gi, address aggregator) external view returns (bool); + /// @dev Per-(aggregator, finalized-batch) settlement weight for a GI, /// read by DINTaskAuditor.claimReward at claim time (BL-10, #125). function aggregatorWeight( @@ -364,6 +370,10 @@ error TA_RewardsNotSettled(); /// settled; claims pay from the snapshot, so a pool penalty could no /// longer be taken without underfunding them. error TA_RewardsAlreadySettled(); +/// @dev registerDINAuditor was called by an address already registered as an +/// aggregator for the same GI. One address can't hold both roles in a GI +/// (issue No. 180); a second address with its own stake still can. +error TA_DualRoleNotAllowed(); /// @dev Caller already claimed their reward for this GI. error TA_RewardAlreadyClaimed(); /// @dev Caller earned no reward for this GI (not a participant, or scored zero). diff --git a/foundry/src/DINTaskAuditor.sol b/foundry/src/DINTaskAuditor.sol index c465286f..342222a0 100644 --- a/foundry/src/DINTaskAuditor.sol +++ b/foundry/src/DINTaskAuditor.sol @@ -92,7 +92,7 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { mapping(uint256 => uint256) public giTotalAuditWeight; /// @notice Per-auditor hasAuditedLM count for a GI, incremented in revealAuditScore. - mapping(uint256 => mapping(address => uint256)) public auditorGIWeight; + mapping(uint256 => mapping(address => uint256)) internal auditorGIWeight; /// @notice Whether a participant has already called claimReward for a GI. mapping(uint256 => mapping(address => bool)) public rewardClaimed; @@ -100,7 +100,7 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { uint MAX_LM_SUBMISSIONS = 10000; uint256 public constant MAX_REGISTERED_AUDITORS = 300; - mapping(uint => address[]) public dinAuditors; + mapping(uint => address[]) internal dinAuditors; // Track if an address is registered for a given _GI mapping(uint => mapping(address => bool)) public isRegisteredAuditor; @@ -224,7 +224,7 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { bytes testDataCID; // encryptedCID = AES-256-GCM(K, rawCID || Sign(ownerSK, rawCID)) } - mapping(uint256 => AuditBatch[]) public auditBatches; + mapping(uint256 => AuditBatch[]) internal auditBatches; mapping(uint => mapping(uint => mapping(address => bool))) public isBatchAuditor; @@ -259,7 +259,7 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { mapping(uint256 => mapping(uint => mapping(address => mapping(uint => bool)))) // GI // batchId // auditor // modelIndex // has committed public hasCommittedLM; - mapping(uint256 => bool) public Is_testdataCIDs_Assigned; + mapping(uint256 => bool) internal Is_testdataCIDs_Assigned; // §2b: per-validator encrypted test-data key mapping. GI => batchId => // auditor => that auditor's copy of the test-data decryption key, @@ -487,21 +487,22 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { emit RewardDeposited(gi, msg.sender, amount); } + /// @dev Bounds- and id-checked batch lookup shared by the test-data paths. + function _batchAt(uint256 gi, uint256 batchId) internal view returns (AuditBatch storage batch) { + if (batchId >= auditBatches[gi].length) revert TA_BatchDoesNotExist(); + batch = auditBatches[gi][batchId]; + if (batch.batchId != batchId) revert TA_BatchIDMismatch(); + } + /// @dev Burns 50% of `amount` and forwards 50% to the platform slash-treasury /// (`dinvalidatorStakeContract.slashTreasury()`). Burns both halves when /// the slash-treasury is unset. Increments `treasuryAccrued` for observability. function _burnAndForward(uint256 amount) internal { if (amount == 0) return; uint256 burnAmt = amount / 2; - uint256 fwdAmt = amount - burnAmt; IBurnableDinToken(address(dinToken)).burn(burnAmt); - address treasury = dinvalidatorStakeContract.slashTreasury(); - if (treasury != address(0)) { - dinToken.safeTransfer(treasury, fwdAmt); - } else { - IBurnableDinToken(address(dinToken)).burn(fwdAmt); - } - treasuryAccrued += amount; + treasuryAccrued += burnAmt; + _forwardToTreasury(amount - burnAmt); } /// @dev Forwards `amount` in full to the platform slash-treasury. @@ -663,6 +664,12 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { /// @notice Registers the caller as an auditor for the current GI. /// @dev Caller must be an active validator; duplicate registrations revert. /// The coordinator's GI state must be DINauditorsRegistrationStarted. + /// An address already registered as an aggregator for this GI is + /// refused (TA_DualRoleNotAllowed, issue No. 180). The check lives + /// here because aggregator registration (states 6-7) always comes + /// before auditor registration (8-9), so the coordinator side could + /// never see an auditor yet. It is per address: a second address with + /// its own stake can still register. /// @param _GI Current GI index. function registerDINAuditor(uint _GI) public onlyCurrentGI(_GI) { if ( @@ -671,6 +678,8 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { ) revert TA_AuditorRegistrationNotOpen(); if (isRegisteredAuditor[_GI][msg.sender]) revert TA_AuditorAlreadyRegistered(); + if (dintaskcoordinatorContract.isDINAggregator(_GI, msg.sender)) + revert TA_DualRoleNotAllowed(); if (dinAuditors[_GI].length >= MAX_REGISTERED_AUDITORS) revert TA_RegistrationCapReached(); @@ -986,24 +995,31 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { bytes[] calldata encryptedKeys, bytes32 commitment ) external onlyOwner onlyCurrentGI(gi) { - if (batchId >= auditBatches[gi].length) revert TA_BatchDoesNotExist(); - AuditBatch storage batch = auditBatches[gi][batchId]; - if (batch.batchId != batchId) revert TA_BatchIDMismatch(); + AuditBatch storage batch = _batchAt(gi, batchId); if (encryptedKeys.length != batch.auditors.length) revert TA_EncryptedKeyCountMismatch(); if (testDataDisputes[gi][batchId].pendingReassignment) revert TA_BatchPendingReassignment(); - batch.testDataCID = testDataCID; + _storeTestData(gi, batchId, batch, testDataCID, encryptedKeys, commitment); + } + /// @dev Shared tail of assignAuditTestDataset / reassignAuditTestDataset. + function _storeTestData( + uint256 gi, + uint256 batchId, + AuditBatch storage batch, + bytes calldata testDataCID, + bytes[] calldata encryptedKeys, + bytes32 commitment + ) internal { + batch.testDataCID = testDataCID; for (uint256 i = 0; i < encryptedKeys.length; i++) { if (dinvalidatorStakeContract.getEncryptionKey(batch.auditors[i]).length == 0) revert TA_AuditorEncryptionKeyNotRegistered(); encryptedTestDataKey[gi][batchId][batch.auditors[i]] = encryptedKeys[i]; } - testDataCommitments[gi][batchId] = commitment; - emit EncryptedTestDataKeysAssigned(gi, batchId, testDataCID, encryptedKeys.length); emit TestDataCommitmentStored(gi, batchId, commitment); } @@ -1605,24 +1621,13 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { bytes[] calldata newEncryptedKeys, bytes32 newCommitment ) external onlyOwner onlyCurrentGI(gi) { - if (batchId >= auditBatches[gi].length) revert TA_BatchDoesNotExist(); - AuditBatch storage batch = auditBatches[gi][batchId]; - if (batch.batchId != batchId) revert TA_BatchIDMismatch(); + AuditBatch storage batch = _batchAt(gi, batchId); if (!testDataDisputes[gi][batchId].pendingReassignment) revert TA_NoActiveDispute(); if (newEncryptedKeys.length != batch.auditors.length) revert TA_EncryptedKeyCountMismatch(); testDataDisputes[gi][batchId].pendingReassignment = false; - batch.testDataCID = newTestDataCID; - for (uint256 i = 0; i < newEncryptedKeys.length; i++) { - if (dinvalidatorStakeContract.getEncryptionKey(batch.auditors[i]).length == 0) - revert TA_AuditorEncryptionKeyNotRegistered(); - encryptedTestDataKey[gi][batchId][batch.auditors[i]] = newEncryptedKeys[i]; - } - testDataCommitments[gi][batchId] = newCommitment; - - emit EncryptedTestDataKeysAssigned(gi, batchId, newTestDataCID, newEncryptedKeys.length); - emit TestDataCommitmentStored(gi, batchId, newCommitment); + _storeTestData(gi, batchId, batch, newTestDataCID, newEncryptedKeys, newCommitment); } } diff --git a/foundry/src/DinValidatorStake.sol b/foundry/src/DinValidatorStake.sol index ed61fba3..12ce88bf 100644 --- a/foundry/src/DinValidatorStake.sol +++ b/foundry/src/DinValidatorStake.sol @@ -118,7 +118,9 @@ contract DinValidatorStake is /// @notice Number of active GI registrations held by each validator across /// all task contracts. Incremented by task contracts (as slashers) - /// at registration time, decremented at endGI time. Used to enforce + /// at registration time, decremented when the model owner calls + /// DINTaskCoordinator.releaseGIRegistrationSlots for an ended GI + /// (endGI itself does not decrement). Used to enforce /// maxConcurrentRegistrationsPerStakeUnit when non-zero. mapping(address => uint256) public activeRegistrationCount; @@ -448,7 +450,10 @@ contract DinValidatorStake is emit UnbondingPeriodUpdated(newPeriod); } - /// @notice Stores per-model stake bounds (not yet enforced — follow-up task). + /// @notice Stores per-model stake bounds. The min bound is enforced as the + /// per-model stake floor in DINTaskCoordinator.registerDINaggregator + /// and DINTaskAuditor.registerDINAuditor (via getModelStakeMin); + /// the max bound is stored but not read on-chain. function setModelStakeBounds( uint256 modelId, uint256 min, @@ -459,7 +464,9 @@ contract DinValidatorStake is emit ModelStakeBoundsUpdated(modelId, min, max); } - /// @notice Stores the concurrent-registration cap per stake unit (not yet enforced). + /// @notice Stores the concurrent-registration cap per stake unit, enforced + /// in DINTaskCoordinator.registerDINaggregator and + /// DINTaskAuditor.registerDINAuditor when non-zero (0 = no cap). function setMaxConcurrentRegistrationsPerStakeUnit( uint256 value ) external onlyOwner { @@ -467,7 +474,8 @@ contract DinValidatorStake is emit MaxConcurrentRegistrationsPerStakeUnitUpdated(value); } - /// @notice Returns the per-model minimum stake floor set by the model owner. + /// @notice Returns the per-model minimum stake floor set by the owner + /// (DIN-Representative) via setModelStakeBounds. /// @param modelId Model registry ID to query. function getModelStakeMin(uint256 modelId) external view returns (uint256) { return modelMinStakeBounds[modelId].min; diff --git a/foundry/test/StakingEnforcement.t.sol b/foundry/test/StakingEnforcement.t.sol index 5cbc1c0e..38cf069d 100644 --- a/foundry/test/StakingEnforcement.t.sol +++ b/foundry/test/StakingEnforcement.t.sol @@ -23,7 +23,8 @@ import { TC_ConcurrentRegistrationCapReached, TC_WrongGI, TA_StakeBelowModelFloor, - TA_ConcurrentRegistrationCapReached + TA_ConcurrentRegistrationCapReached, + TA_DualRoleNotAllowed } from "../src/DINShared.sol"; import {auditCommitHash} from "./utils/AuditCommitHash.sol"; @@ -485,4 +486,76 @@ contract StakingEnforcementTest is Test { tc.releaseGIRegistrationSlots(1); vm.stopPrank(); } + + // ── one address can't hold both roles in a GI (issue No. 180) ──────────── + + /// @dev An aggregator for GI 1 can't also register as an auditor for GI 1. + function test_registerDINAuditor_aggregatorSameGI_reverts() public { + _stake(agg1, MIN_STAKE_AMOUNT * 2); + _advanceToAggregatorRegistration(); + vm.prank(agg1); + tc.registerDINaggregator(1); + + vm.startPrank(modelOwner); + tc.closeDINaggregatorsRegistration(1); + tc.startDINauditorsRegistration(1); + vm.stopPrank(); + + vm.prank(agg1); + vm.expectRevert(TA_DualRoleNotAllowed.selector); + ta.registerDINAuditor(1); + assertFalse(ta.isRegisteredAuditor(1, agg1)); + assertEq(stake.activeRegistrationCount(agg1), 1, "only the aggregator slot is held"); + } + + /// @dev The guard is per GI: GI 1's aggregator may audit in GI 2. + function test_registerDINAuditor_aggregatorOfEarlierGI_registers() public { + _runFullGIToEnded(); // agg1 is a GI 1 aggregator + + address funder = makeAddr("funder_gi2"); + uint256 poolEth = 0.001 ether; + vm.deal(funder, poolEth + 1 ether); + vm.prank(funder); + coordinator.depositAndMint{value: poolEth}(); + uint256 pool = token.balanceOf(funder); + vm.startPrank(funder); + token.approve(address(ta), pool); + ta.depositRewards(2, pool); + vm.stopPrank(); + + vm.startPrank(modelOwner); + tc.startGI(2); + tc.startDINaggregatorsRegistration(2); + tc.closeDINaggregatorsRegistration(2); + tc.startDINauditorsRegistration(2); + vm.stopPrank(); + + assertTrue(tc.isDINAggregator(1, agg1)); + assertFalse(tc.isDINAggregator(2, agg1)); + vm.prank(agg1); + ta.registerDINAuditor(2); // must not revert + assertTrue(ta.isRegisteredAuditor(2, agg1)); + } + + /// @dev Documents the guard's limit: it is per address, so the same + /// operator with a second address and its own stake can still take + /// the auditor role (Sybil). The defence is that it costs a second + /// stake and is visible on-chain. + function test_registerDINAuditor_secondAddressWithOwnStake_registers() public { + _stake(agg1, MIN_STAKE_AMOUNT); + _stake(aud1, MIN_STAKE_AMOUNT); // the operator's second address + _advanceToAggregatorRegistration(); + vm.prank(agg1); + tc.registerDINaggregator(1); + + vm.startPrank(modelOwner); + tc.closeDINaggregatorsRegistration(1); + tc.startDINauditorsRegistration(1); + vm.stopPrank(); + + vm.prank(aud1); + ta.registerDINAuditor(1); // must not revert + assertTrue(ta.isRegisteredAuditor(1, aud1)); + assertTrue(tc.isDINAggregator(1, agg1)); + } }