diff --git a/foundry/src/DINTaskAuditor.sol b/foundry/src/DINTaskAuditor.sol index ea11dfb..833b793 100644 --- a/foundry/src/DINTaskAuditor.sol +++ b/foundry/src/DINTaskAuditor.sol @@ -687,16 +687,19 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { // Per-model stake floor: enforced when the model owner has set a non-zero bound. uint256 floorMin = dinvalidatorStakeContract.getModelStakeMin(modelId); - if (floorMin > 0 && dinvalidatorStakeContract.getStake(msg.sender) < floorMin) - revert TA_StakeBelowModelFloor(); - // Concurrent-registration cap: enforced when the DAO has set a non-zero value. uint256 capPerUnit = dinvalidatorStakeContract.maxConcurrentRegistrationsPerStakeUnit(); - if (capPerUnit > 0) { - uint256 maxAllowed = (dinvalidatorStakeContract.getStake(msg.sender) / - dinvalidatorStakeContract.minStake()) * capPerUnit; - if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) - revert TA_ConcurrentRegistrationCapReached(); + if (floorMin > 0 || capPerUnit > 0) { + // Single getStake() read shared by both checks below (issue #249 finding 1). + uint256 stake = dinvalidatorStakeContract.getStake(msg.sender); + if (floorMin > 0 && stake < floorMin) + revert TA_StakeBelowModelFloor(); + if (capPerUnit > 0) { + uint256 maxAllowed = (stake / + dinvalidatorStakeContract.minStake()) * capPerUnit; + if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) + revert TA_ConcurrentRegistrationCapReached(); + } } dinAuditors[_GI].push(msg.sender); @@ -792,22 +795,25 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { uint _GI ) internal view returns (address[] memory activePool) { address[] storage registeredPool = dinAuditors[_GI]; + uint len = registeredPool.length; + // issue #249 finding 2: cache each isValidatorActive result from the + // counting pass instead of re-querying it in the fill pass below. + bool[] memory active = new bool[](len); uint activeCount; - for (uint i = 0; i < registeredPool.length; i++) { - if ( - dinvalidatorStakeContract.isValidatorActive(registeredPool[i]) - ) { + for (uint i = 0; i < len; i++) { + bool isActive = dinvalidatorStakeContract.isValidatorActive(registeredPool[i]); + active[i] = isActive; + if (isActive) { activeCount++; } } activePool = new address[](activeCount); uint ptr; - for (uint i = 0; i < registeredPool.length; i++) { - address auditor = registeredPool[i]; - if (dinvalidatorStakeContract.isValidatorActive(auditor)) { - activePool[ptr++] = auditor; + for (uint i = 0; i < len; i++) { + if (active[i]) { + activePool[ptr++] = registeredPool[i]; } } } @@ -1231,9 +1237,14 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { for (uint b = 0; b < batches; b++) { AuditBatch storage batch = auditBatches[_GI][b]; + // issue #249 finding 3: both lengths are loop-invariant per + // batch; cache instead of re-reading storage on every iteration + // (modelIndexes.length) or per-model (auditors.length x3). + uint modelIndexCount = batch.modelIndexes.length; + uint auditorCount = batch.auditors.length; // For each model assigned to this batch - for (uint m = 0; m < batch.modelIndexes.length; m++) { + for (uint m = 0; m < modelIndexCount; m++) { uint modelIndex = batch.modelIndexes[m]; if (modelIndex >= submissions.length) { // Defensive: skip if bad index @@ -1250,15 +1261,11 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { // Collect scores from auditors who actually voted. Bounded by // batch.auditors.length (params.auditorsPerBatch, demo: 3, // spec: up to 10), so an in-memory sort here is cheap. - uint256[] memory votedScores = new uint256[]( - batch.auditors.length - ); - address[] memory votedAuditors = new address[]( - batch.auditors.length - ); + uint256[] memory votedScores = new uint256[](auditorCount); + address[] memory votedAuditors = new address[](auditorCount); uint votes; - for (uint a = 0; a < batch.auditors.length; a++) { + for (uint a = 0; a < auditorCount; a++) { address auditor = batch.auditors[a]; if (hasAuditedLM[_GI][b][auditor][modelIndex]) { votedScores[votes] = auditScores[_GI][b][auditor][ @@ -1359,12 +1366,18 @@ contract DINTaskAuditor is Ownable, ReentrancyGuardTransient { for (uint b = 0; b < batchCount; b++) { AuditBatch storage batch = auditBatches[_GI][b]; - for (uint a = 0; a < batch.auditors.length; a++) { + // issue #249 finding 3: both lengths are loop-invariant per + // batch; cache instead of re-reading storage on every iteration + // of the loops they bound (modelIndexes.length was re-read up to + // auditorCount x modelIndexCount times without this). + uint auditorCount = batch.auditors.length; + uint modelIndexCount = batch.modelIndexes.length; + for (uint a = 0; a < auditorCount; a++) { address auditor = batch.auditors[a]; bool missedVote = false; bool exceededDeviation = false; - for (uint m = 0; m < batch.modelIndexes.length; m++) { + for (uint m = 0; m < modelIndexCount; m++) { uint modelIndex = batch.modelIndexes[m]; if (!hasAuditedLM[_GI][b][auditor][modelIndex]) { missedVote = true; diff --git a/foundry/src/DINTaskCoordinator.sol b/foundry/src/DINTaskCoordinator.sol index 56aca44..fee3720 100644 --- a/foundry/src/DINTaskCoordinator.sol +++ b/foundry/src/DINTaskCoordinator.sol @@ -412,17 +412,20 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { // Per-model stake floor: enforced when the model owner has set a non-zero bound. uint256 floorMin = dinvalidatorStakeContract.getModelStakeMin(modelId); - if (floorMin > 0 && dinvalidatorStakeContract.getStake(msg.sender) < floorMin) - revert TC_StakeBelowModelFloor(); - // Concurrent-registration cap: enforced when the DAO has set a non-zero value. // Formula: each MIN_STAKE unit of stake allows capPerUnit concurrent registrations. uint256 capPerUnit = dinvalidatorStakeContract.maxConcurrentRegistrationsPerStakeUnit(); - if (capPerUnit > 0) { - uint256 maxAllowed = (dinvalidatorStakeContract.getStake(msg.sender) / - dinvalidatorStakeContract.minStake()) * capPerUnit; - if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) - revert TC_ConcurrentRegistrationCapReached(); + if (floorMin > 0 || capPerUnit > 0) { + // Single getStake() read shared by both checks below (issue #249 finding 1). + uint256 stake = dinvalidatorStakeContract.getStake(msg.sender); + if (floorMin > 0 && stake < floorMin) + revert TC_StakeBelowModelFloor(); + if (capPerUnit > 0) { + uint256 maxAllowed = (stake / + dinvalidatorStakeContract.minStake()) * capPerUnit; + if (dinvalidatorStakeContract.activeRegistrationCount(msg.sender) >= maxAllowed) + revert TC_ConcurrentRegistrationCapReached(); + } } // Add to list and mark as registered @@ -697,22 +700,25 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { uint _GI ) internal view returns (address[] memory activePool) { address[] storage registeredPool = dinAggregators[_GI]; + uint len = registeredPool.length; + // issue #249 finding 2: cache each isValidatorActive result from the + // counting pass instead of re-querying it in the fill pass below. + bool[] memory active = new bool[](len); uint activeCount; - for (uint i = 0; i < registeredPool.length; i++) { - if ( - dinvalidatorStakeContract.isValidatorActive(registeredPool[i]) - ) { + for (uint i = 0; i < len; i++) { + bool isActive = dinvalidatorStakeContract.isValidatorActive(registeredPool[i]); + active[i] = isActive; + if (isActive) { activeCount++; } } activePool = new address[](activeCount); uint ptr; - for (uint i = 0; i < registeredPool.length; i++) { - address validator = registeredPool[i]; - if (dinvalidatorStakeContract.isValidatorActive(validator)) { - activePool[ptr++] = validator; + for (uint i = 0; i < len; i++) { + if (active[i]) { + activePool[ptr++] = registeredPool[i]; } } } @@ -948,21 +954,26 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_NotReadyToFinalizeT1(); Tier1Batch[] storage batches = tier1Batches[_GI]; + uint batchCount = batches.length; - for (uint i = 0; i < batches.length; i++) { + for (uint i = 0; i < batchCount; i++) { Tier1Batch storage b = batches[i]; + // issue #249 finding 3: batchId/aggregators.length are + // loop-invariant per batch; cache instead of re-reading storage. + uint batchId = b.batchId; + uint aggregatorCount = b.aggregators.length; // Determine the CID with the most votes bytes32 winningCID = ""; uint maxVotes = 0; uint submissionCount = 0; - for (uint j = 0; j < b.aggregators.length; j++) { + for (uint j = 0; j < aggregatorCount; j++) { address aggregator = b.aggregators[j]; - if (t1Submitted[_GI][b.batchId][aggregator]) { + if (t1Submitted[_GI][batchId][aggregator]) { submissionCount++; - bytes32 cid = t1SubmissionCID[_GI][b.batchId][aggregator]; - uint votes = t1Votes[_GI][b.batchId][cid]; + bytes32 cid = t1SubmissionCID[_GI][batchId][aggregator]; + uint votes = t1Votes[_GI][batchId][cid]; if (votes > maxVotes) { maxVotes = votes; winningCID = cid; @@ -985,8 +996,8 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_InsufficientSubmissions(); b.finalized = true; b.finalCID = winningCID; - tier1FinalizedAt[_GI][b.batchId] = uint64(block.timestamp); - emit T1BatchFinalized(_GI, b.batchId, winningCID); + tier1FinalizedAt[_GI][batchId] = uint64(block.timestamp); + emit T1BatchFinalized(_GI, batchId, winningCID); } _setGIstate(GIstates.T1AggregationDone); @@ -1098,21 +1109,26 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_NotReadyToFinalizeT2(); Tier2Batch[] storage batches = tier2Batches[_GI]; + uint batchCount = batches.length; - for (uint i = 0; i < batches.length; i++) { + for (uint i = 0; i < batchCount; i++) { Tier2Batch storage b = batches[i]; + // issue #249 finding 3: batchId/aggregators.length are + // loop-invariant per batch; cache instead of re-reading storage. + uint batchId = b.batchId; + uint aggregatorCount = b.aggregators.length; // Determine the CID with the most votes bytes32 winningCID = ""; uint maxVotes = 0; uint submissionCount = 0; - for (uint j = 0; j < b.aggregators.length; j++) { + for (uint j = 0; j < aggregatorCount; j++) { address aggregator = b.aggregators[j]; - if (t2Submitted[_GI][b.batchId][aggregator]) { + if (t2Submitted[_GI][batchId][aggregator]) { submissionCount++; - bytes32 cid = t2SubmissionCID[_GI][b.batchId][aggregator]; - uint votes = t2Votes[_GI][b.batchId][cid]; + bytes32 cid = t2SubmissionCID[_GI][batchId][aggregator]; + uint votes = t2Votes[_GI][batchId][cid]; if (votes > maxVotes) { maxVotes = votes; winningCID = cid; @@ -1133,7 +1149,7 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { revert TC_InsufficientSubmissions(); b.finalized = true; b.finalCID = winningCID; - tier2FinalizedAt[_GI][b.batchId] = uint64(block.timestamp); + tier2FinalizedAt[_GI][batchId] = uint64(block.timestamp); emit T2Finalized(_GI, winningCID); } @@ -1168,12 +1184,14 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { uint256 s2Amount = (minStakeAmt * s2SlashFractionBps) / 10_000; Tier1Batch[] storage t1batches = tier1Batches[_GI]; - for (uint i = 0; i < t1batches.length; i++) { + uint t1Count = t1batches.length; + for (uint i = 0; i < t1Count; i++) { Tier1Batch storage b = t1batches[i]; _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, false); } Tier2Batch[] storage t2batches = tier2Batches[_GI]; - for (uint i = 0; i < t2batches.length; i++) { + uint t2Count = t2batches.length; + for (uint i = 0; i < t2Count; i++) { Tier2Batch storage b = t2batches[i]; _slashBatch(_GI, b.batchId, b.aggregators, b.finalCID, minStakeAmt, s2Amount, true); } @@ -1205,7 +1223,8 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { noSubmission = "AGG_T2_NO_SUBMISSION"; badConsensus = "AGG_T2_BAD_CONSENSUS"; } - for (uint j = 0; j < aggs.length; j++) { + uint aggCount = aggs.length; + for (uint j = 0; j < aggCount; j++) { address aggregator = aggs[j]; bool submitted = t2 ? t2Submitted[_GI][batchId][aggregator] @@ -1296,7 +1315,8 @@ contract DINTaskCoordinator is Ownable, ReentrancyGuardTransient { registrationSlotsReleased[_GI] = true; address[] storage aggs = dinAggregators[_GI]; - for (uint256 i = 0; i < aggs.length; i++) { + uint256 aggCount = aggs.length; + for (uint256 i = 0; i < aggCount; i++) { dinvalidatorStakeContract.decrementActiveRegistration(aggs[i]); } dinTaskAuditorContract.decrementAuditorRegistrations(_GI); diff --git a/foundry/src/DinValidatorStake.sol b/foundry/src/DinValidatorStake.sol index 3685634..121c5e5 100644 --- a/foundry/src/DinValidatorStake.sol +++ b/foundry/src/DinValidatorStake.sol @@ -836,22 +836,26 @@ contract DinValidatorStake is } function _syncValidatorStatus(ValidatorInfo storage validator) internal { - if (validator.status == ValidatorStatus.Blacklisted) { + // issue #249 finding 4: cache status/activeStake instead of + // re-reading each from storage. + ValidatorStatus status = validator.status; + if (status == ValidatorStatus.Blacklisted) { return; } if ( - validator.status == ValidatorStatus.Jailed && + status == ValidatorStatus.Jailed && validator.jailedUntil > block.timestamp ) { return; } + uint256 activeStake = validator.activeStake; if (validator.pendingWithdrawals > 0) { validator.status = ValidatorStatus.Exiting; - } else if (validator.activeStake >= MIN_STAKE) { + } else if (activeStake >= MIN_STAKE) { validator.status = ValidatorStatus.Active; - } else if (validator.activeStake > 0) { + } else if (activeStake > 0) { validator.status = ValidatorStatus.Exiting; } else { validator.status = ValidatorStatus.None; diff --git a/foundry/test/StakingEnforcement.t.sol b/foundry/test/StakingEnforcement.t.sol index f0d1cdb..aa45ea9 100644 --- a/foundry/test/StakingEnforcement.t.sol +++ b/foundry/test/StakingEnforcement.t.sol @@ -22,6 +22,7 @@ import { TC_StakeBelowModelFloor, TC_ConcurrentRegistrationCapReached, TC_WrongGI, + TC_GINotEnded, TC_SlotsAlreadyReleased, TA_StakeBelowModelFloor, TA_ConcurrentRegistrationCapReached, @@ -490,6 +491,15 @@ contract StakingEnforcementTest is Test { vm.stopPrank(); } + function test_releaseGIRegistrationSlots_revertsWhenGINotEnded() public { + // GI 1 is still in progress (never driven to GIended) -- neither + // "_GI < current GI" nor "_GI == current GI && GIstate == GIended" + // holds, so the call must revert rather than release slots early. + vm.prank(modelOwner); + vm.expectRevert(TC_GINotEnded.selector); + tc.releaseGIRegistrationSlots(1); + } + // ── 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.