Merge pull request #62 from liquity/fix-audit-and-invariants

Fix audit and invariants

Invariants.MD

@@ -0,0 +1,10 @@
+Snapshot Solvency
+
+            uint256 claim = _votesForInitiativeSnapshot.votes * boldAccrued / _votesSnapshot.votes;
+For each initiative this is what the value is
+If the initiative is "Claimable" this is what it receives
+The call never reverts
+The sum of claims is less than the boldAccrued
+
+Veto consistency
+

ToFix.MD

@@ -0,0 +1,31 @@
+- Add properties check to ensure that the math is sound <- HUGE, let's add it now
+
+A vote is: User TS * Votes
+So an allocation should use that
+We need to remove the data from the valid allocation
+And not from a random one
+
+I think the best test is to simply store the contribution done
+And see whether removing it is idempotent
+
+We would need a ton of work to make it even better
+
+
+Specifically, if a user removes their votes, we need to see that reflect correctly
+Because that's key
+
+- From there, try fixing with a reset on deposit and withdraw
+
+- Add a test that checks every: initiative, user allocation, ensure they are zero after a deposit and a withdrawal
+- Add a test that checks every: X, ensure they use the correct TS
+
+- From there, reason around the deeper rounding errors
+
+
+
+Optimizations
+Put the data in the storage
+Remove all castings that are not safe
+Invariant test it
+
+--

echidna.yaml

@@ -1,8 +1,10 @@
-testMode: "assertion"
-prefix: "crytic_"
+testMode: "property"
+prefix: "optimize_"
 coverage: true
 corpusDir: "echidna"
 balanceAddr: 0x1043561a8829300000
 balanceContract: 0x1043561a8829300000
 filterFunctions: []
-cryticArgs: ["--foundry-compile-all"]
+cryticArgs: ["--foundry-compile-all"]
+
+shrinkLimit: 100000

src/BribeInitiative.sol

@@ -45,19 +45,19 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
     }
 
     /// @inheritdoc IBribeInitiative
-    function totalLQTYAllocatedByEpoch(uint16 _epoch) external view returns (uint88, uint32) {
+    function totalLQTYAllocatedByEpoch(uint16 _epoch) external view returns (uint88, uint120) {
         return _loadTotalLQTYAllocation(_epoch);
     }
 
     /// @inheritdoc IBribeInitiative
-    function lqtyAllocatedByUserAtEpoch(address _user, uint16 _epoch) external view returns (uint88, uint32) {
+    function lqtyAllocatedByUserAtEpoch(address _user, uint16 _epoch) external view returns (uint88, uint120) {
         return _loadLQTYAllocation(_user, _epoch);
     }
 
     /// @inheritdoc IBribeInitiative
     function depositBribe(uint128 _boldAmount, uint128 _bribeTokenAmount, uint16 _epoch) external {
         uint16 epoch = governance.epoch();
-        require(_epoch >= epoch, "BribeInitiative: only-future-epochs");
+        require(_epoch >= epoch, "BribeInitiative: now-or-future-epochs");
 
         Bribe memory bribe = bribeByEpoch[_epoch];
         bribe.boldAmount += _boldAmount;
@@ -70,6 +70,8 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
         bribeToken.safeTransferFrom(msg.sender, address(this), _bribeTokenAmount);
     }
 
+    uint256 constant TIMESTAMP_PRECISION = 1e26;
+
     function _claimBribe(
         address _user,
         uint16 _epoch,
@@ -98,11 +100,24 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
             "BribeInitiative: invalid-prev-total-lqty-allocation-epoch"
         );
 
-        (uint88 totalLQTY, uint32 totalAverageTimestamp) = _decodeLQTYAllocation(totalLQTYAllocation.value);
-        uint240 totalVotes = governance.lqtyToVotes(totalLQTY, block.timestamp, totalAverageTimestamp);
+        (uint88 totalLQTY, uint120 totalAverageTimestamp) = _decodeLQTYAllocation(totalLQTYAllocation.value);
+
+        // NOTE: SCALING!!! | The timestamp will work until type(uint32).max | After which the math will eventually overflow
+        uint120 scaledEpochEnd = (
+            uint120(governance.EPOCH_START()) + uint120(_epoch) * uint120(governance.EPOCH_DURATION())
+        ) * uint120(TIMESTAMP_PRECISION);
+
+        /// @audit User Invariant
+        assert(totalAverageTimestamp <= scaledEpochEnd);
+
+        uint240 totalVotes = governance.lqtyToVotes(totalLQTY, scaledEpochEnd, totalAverageTimestamp);
         if (totalVotes != 0) {
-            (uint88 lqty, uint32 averageTimestamp) = _decodeLQTYAllocation(lqtyAllocation.value);
-            uint240 votes = governance.lqtyToVotes(lqty, block.timestamp, averageTimestamp);
+            (uint88 lqty, uint120 averageTimestamp) = _decodeLQTYAllocation(lqtyAllocation.value);
+
+            /// @audit Governance Invariant
+            assert(averageTimestamp <= scaledEpochEnd);
+
+            uint240 votes = governance.lqtyToVotes(lqty, scaledEpochEnd, averageTimestamp);
             boldAmount = uint256(bribe.boldAmount) * uint256(votes) / uint256(totalVotes);
             bribeTokenAmount = uint256(bribe.bribeTokenAmount) * uint256(votes) / uint256(totalVotes);
         }
@@ -126,13 +141,17 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
             bribeTokenAmount += bribeTokenAmount_;
         }
 
+        // NOTE: Due to rounding errors in the `averageTimestamp` bribes may slightly overpay compared to what they have allocated
+        // We cap to the available amount for this reason
+        // The error should be below 10 LQTY per annum, in the worst case
         if (boldAmount != 0) {
             uint256 max = bold.balanceOf(address(this));
             if (boldAmount > max) {
                 boldAmount = max;
             }
             bold.safeTransfer(msg.sender, boldAmount);
         }
+
         if (bribeTokenAmount != 0) {
             uint256 max = bribeToken.balanceOf(address(this));
             if (bribeTokenAmount > max) {
@@ -148,10 +167,10 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
     /// @inheritdoc IInitiative
     function onUnregisterInitiative(uint16) external virtual override onlyGovernance {}
 
-    function _setTotalLQTYAllocationByEpoch(uint16 _epoch, uint88 _lqty, uint32 _averageTimestamp, bool _insert)
+    function _setTotalLQTYAllocationByEpoch(uint16 _epoch, uint88 _lqty, uint120 _averageTimestamp, bool _insert)
         private
     {
-        uint224 value = (uint224(_lqty) << 32) | _averageTimestamp;
+        uint224 value = _encodeLQTYAllocation(_lqty, _averageTimestamp);
         if (_insert) {
             totalLQTYAllocationByEpoch.insert(_epoch, value, 0);
         } else {
@@ -164,10 +183,10 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
         address _user,
         uint16 _epoch,
         uint88 _lqty,
-        uint32 _averageTimestamp,
+        uint120 _averageTimestamp,
         bool _insert
     ) private {
-        uint224 value = (uint224(_lqty) << 32) | _averageTimestamp;
+        uint224 value = _encodeLQTYAllocation(_lqty, _averageTimestamp);
         if (_insert) {
             lqtyAllocationByUserAtEpoch[_user].insert(_epoch, value, 0);
         } else {
@@ -176,20 +195,20 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
         emit ModifyLQTYAllocation(_user, _epoch, _lqty, _averageTimestamp);
     }
 
-    function _encodeLQTYAllocation(uint88 _lqty, uint32 _averageTimestamp) private pure returns (uint224) {
+    function _encodeLQTYAllocation(uint88 _lqty, uint120 _averageTimestamp) private pure returns (uint224) {
         return EncodingDecodingLib.encodeLQTYAllocation(_lqty, _averageTimestamp);
     }
 
-    function _decodeLQTYAllocation(uint224 _value) private pure returns (uint88, uint32) {
+    function _decodeLQTYAllocation(uint224 _value) private pure returns (uint88, uint120) {
         return EncodingDecodingLib.decodeLQTYAllocation(_value);
     }
 
-    function _loadTotalLQTYAllocation(uint16 _epoch) private view returns (uint88, uint32) {
+    function _loadTotalLQTYAllocation(uint16 _epoch) private view returns (uint88, uint120) {
         require(_epoch <= governance.epoch(), "No future Lookup");
         return _decodeLQTYAllocation(totalLQTYAllocationByEpoch.items[_epoch].value);
     }
 
-    function _loadLQTYAllocation(address _user, uint16 _epoch) private view returns (uint88, uint32) {
+    function _loadLQTYAllocation(address _user, uint16 _epoch) private view returns (uint88, uint120) {
         require(_epoch <= governance.epoch(), "No future Lookup");
         return _decodeLQTYAllocation(lqtyAllocationByUserAtEpoch[_user].items[_epoch].value);
     }

src/CurveV2GaugeRewards.sol

@@ -26,16 +26,23 @@ contract CurveV2GaugeRewards is BribeInitiative {
         _depositIntoGauge(_bold);
     }
 
+    // TODO: If this is capped, we may need to donate here, so cap it here as well
     function _depositIntoGauge(uint256 amount) internal {
+        uint256 total = amount + remainder;
+
         // For small donations queue them into the contract
-        if (amount < duration * 1000) {
+        if (total < duration * 1000) {
             remainder += amount;
             return;
         }
 
-        uint256 total = amount + remainder;
         remainder = 0;
 
+        uint256 available = bold.balanceOf(address(this));
+        if (available < total) {
+            total = available; // Cap due to rounding error causing a bit more bold being given away
+        }
+
         bold.approve(address(gauge), total);
         gauge.deposit_reward_token(address(bold), total, duration);