gas(contracts): cache repeated storage/external reads in hot paths

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;

foundry/src/DINTaskCoordinator.sol

@@ -1,3 +1,3 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.28;
 
@@ -412,17 +412,20 @@
 
         // 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 @@
         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 @@
             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 @@
                 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 @@
             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 @@
                 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 @@
         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 @@
             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 @@
         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);

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;

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.