Merge pull request #67 from liquity/fix-temp-invariant-align-bribes

Fix temp invariant align bribes

Author: GalloDaSballo

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
+

echidna.yaml

@@ -1,5 +1,5 @@
-testMode: "assertion"
-prefix: "crytic_"
+testMode: "optimization"
+prefix: "optimize_"
 coverage: true
 corpusDir: "echidna"
 balanceAddr: 0x1043561a8829300000

src/BribeInitiative.sol

@@ -99,10 +99,27 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
         );
 
         (uint88 totalLQTY, uint32 totalAverageTimestamp) = _decodeLQTYAllocation(totalLQTYAllocation.value);
-        uint240 totalVotes = governance.lqtyToVotes(totalLQTY, block.timestamp, totalAverageTimestamp);
+        // WHAT HAPPENS IF WE ENFORCE EPOCH AT END?
+        // THEN WE LINEARLY TRANSLATE TO EPOCH END?
+        // EPOCH_START + epoch * EPOCH_DURATION is the time to claim (I think)
+
+        // Since epoch 1 starts at Epoch Start, epoch * Duration goes to the end of 
+        // But is this safe vs last second of the epoch?
+        // I recall having a similar issue already with Velodrome
+        uint32 epochEnd = uint32(governance.EPOCH_START()) + uint32(_epoch) * uint32(governance.EPOCH_DURATION());
+
+        /// @audit User Invariant
+        assert(totalAverageTimestamp <= epochEnd); /// NOTE: Tests break because they are not realistic
+        
+
+        uint240 totalVotes = governance.lqtyToVotes(totalLQTY, epochEnd, totalAverageTimestamp);
         if (totalVotes != 0) {
             (uint88 lqty, uint32 averageTimestamp) = _decodeLQTYAllocation(lqtyAllocation.value);
-            uint240 votes = governance.lqtyToVotes(lqty, block.timestamp, averageTimestamp);
+
+            /// @audit Governance Invariant
+            assert(averageTimestamp <= epochEnd); /// NOTE: Tests break because they are not realistic
+
+            uint240 votes = governance.lqtyToVotes(lqty, epochEnd, averageTimestamp);
             boldAmount = uint256(bribe.boldAmount) * uint256(votes) / uint256(totalVotes);
             bribeTokenAmount = uint256(bribe.bribeTokenAmount) * uint256(votes) / uint256(totalVotes);
         }

src/Governance.sol

@@ -843,10 +843,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
     /// @inheritdoc IGovernance
     function claimForInitiative(address _initiative) external nonReentrant returns (uint256) {
+        // Accrue and update state
         (VoteSnapshot memory votesSnapshot_,) = _snapshotVotes();
         (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) =
             _snapshotVotesForInitiative(_initiative);
 
+        // Compute values on accrued state
         (InitiativeStatus status,, uint256 claimableAmount) =
             getInitiativeState(_initiative, votesSnapshot_, votesForInitiativeSnapshot_, initiativeState);
 
@@ -862,6 +864,14 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
         /// If `lastEpochClaim` is older than epoch() - 1 it means the initiative couldn't claim any rewards this epoch
         initiativeStates[_initiative].lastEpochClaim = epoch() - 1;
 
+        // @audit INVARIANT, because of rounding errors the system can overpay
+        /// We upscale the timestamp to reduce the impact of the loss
+        /// However this is still possible
+        uint256 available = bold.balanceOf(address(this));
+        if(claimableAmount > available) {
+            claimableAmount = available;
+        }
+
         bold.safeTransfer(_initiative, claimableAmount);
 
         emit ClaimForInitiative(_initiative, claimableAmount, votesSnapshot_.forEpoch);

test/BribeInitiativeAllocate.t.sol

@@ -159,9 +159,12 @@ contract BribeInitiativeAllocateTest is Test {
 
     function test_onAfterAllocateLQTY_newEpoch_NoVetoToVeto() public {
         governance.setEpoch(1);
+        vm.warp(governance.EPOCH_DURATION()); // warp to end of first epoch
 
         vm.startPrank(address(governance));
 
+        // set user2 allocations like governance would using onAfterAllocateLQTY at epoch 1 
+        // sets avgTimestamp to current block.timestamp
         {
             IGovernance.UserState memory userState =
                 IGovernance.UserState({allocatedLQTY: 1e18, averageStakingTimestamp: uint32(block.timestamp)});
@@ -184,6 +187,8 @@ contract BribeInitiativeAllocateTest is Test {
             assertEq(userAverageTimestamp, uint32(block.timestamp));
         }
 
+        // set user2 allocations like governance would using onAfterAllocateLQTY at epoch 1
+        // sets avgTimestamp to current block.timestamp 
         {
             IGovernance.UserState memory userState =
                 IGovernance.UserState({allocatedLQTY: 1e18, averageStakingTimestamp: uint32(block.timestamp)});
@@ -196,6 +201,7 @@ contract BribeInitiativeAllocateTest is Test {
                 lastEpochClaim: 0
             });
             bribeInitiative.onAfterAllocateLQTY(governance.epoch(), user2, userState, allocation, initiativeState);
+            
             (uint88 totalLQTYAllocated, uint32 totalAverageTimestamp) =
                 bribeInitiative.totalLQTYAllocatedByEpoch(governance.epoch());
             assertEq(totalLQTYAllocated, 1001e18);
@@ -206,16 +212,20 @@ contract BribeInitiativeAllocateTest is Test {
             assertEq(userAverageTimestamp, uint32(block.timestamp));
         }
 
+        // lusdHolder deposits bribes into the initiative
         vm.startPrank(lusdHolder);
         lqty.approve(address(bribeInitiative), 1000e18);
         lusd.approve(address(bribeInitiative), 1000e18);
         bribeInitiative.depositBribe(1000e18, 1000e18, governance.epoch() + 1);
         vm.stopPrank();
 
         governance.setEpoch(2);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to second epoch ts
 
         vm.startPrank(address(governance));
 
+        // set allocation in initiative for user in epoch 1 
+        // sets avgTimestamp to current block.timestamp 
         {
             IGovernance.UserState memory userState =
                 IGovernance.UserState({allocatedLQTY: 1e18, averageStakingTimestamp: uint32(block.timestamp)});
@@ -238,6 +248,8 @@ contract BribeInitiativeAllocateTest is Test {
             assertEq(userAverageTimestamp, uint32(block.timestamp));
         }
 
+        // set allocation in initiative for user2 in epoch 1
+        // sets avgTimestamp to current block.timestamp 
         {
             IGovernance.UserState memory userState =
                 IGovernance.UserState({allocatedLQTY: 1e18, averageStakingTimestamp: uint32(block.timestamp)});
@@ -260,7 +272,8 @@ contract BribeInitiativeAllocateTest is Test {
             assertEq(userAverageTimestamp, uint32(block.timestamp));
         }
 
-        governance.setEpoch(3);
+        governance.setEpoch(3); 
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to third epoch ts
 
         vm.startPrank(address(user));
 
@@ -269,12 +282,13 @@ contract BribeInitiativeAllocateTest is Test {
         claimData[0].prevLQTYAllocationEpoch = 2;
         claimData[0].prevTotalLQTYAllocationEpoch = 2;
         (uint256 boldAmount, uint256 bribeTokenAmount) = bribeInitiative.claimBribes(claimData);
-        assertEq(boldAmount, 0);
-        assertEq(bribeTokenAmount, 0);
+        assertEq(boldAmount, 0, "boldAmount nonzero");
+        assertEq(bribeTokenAmount, 0, "bribeTokenAmount nonzero");
     }
 
     function test_onAfterAllocateLQTY_newEpoch_VetoToNoVeto() public {
         governance.setEpoch(1);
+        vm.warp(governance.EPOCH_DURATION()); // warp to end of first epoch
 
         vm.startPrank(address(governance));
 
@@ -325,6 +339,7 @@ contract BribeInitiativeAllocateTest is Test {
         assertEq(userAverageTimestampAfterVeto, uint32(block.timestamp));
 
         governance.setEpoch(2);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to second epoch ts
 
         IGovernance.UserState memory userStateNewEpoch =
             IGovernance.UserState({allocatedLQTY: 1, averageStakingTimestamp: uint32(block.timestamp)});
@@ -359,6 +374,7 @@ contract BribeInitiativeAllocateTest is Test {
         vm.startPrank(address(governance));
 
         governance.setEpoch(3);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to third epoch ts
 
         IGovernance.UserState memory userStateNewEpoch3 =
             IGovernance.UserState({allocatedLQTY: 2000e18, averageStakingTimestamp: uint32(block.timestamp)});
@@ -385,6 +401,7 @@ contract BribeInitiativeAllocateTest is Test {
         assertEq(userAverageTimestampNewEpoch3, uint32(block.timestamp));
 
         governance.setEpoch(4);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to fourth epoch ts
 
         vm.startPrank(address(user));
 
@@ -509,6 +526,7 @@ contract BribeInitiativeAllocateTest is Test {
         vm.stopPrank();
 
         governance.setEpoch(1);
+        vm.warp(governance.EPOCH_DURATION()); // warp to end of first epoch
 
         vm.startPrank(address(governance));
 
@@ -585,6 +603,7 @@ contract BribeInitiativeAllocateTest is Test {
         }
 
         governance.setEpoch(2);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to second epoch ts
 
         vm.startPrank(address(user));
 
@@ -603,6 +622,7 @@ contract BribeInitiativeAllocateTest is Test {
         vm.stopPrank();
 
         governance.setEpoch(1);
+        vm.warp(governance.EPOCH_DURATION()); // warp to end of first epoch
 
         vm.startPrank(address(governance));
 
@@ -679,6 +699,7 @@ contract BribeInitiativeAllocateTest is Test {
         }
 
         governance.setEpoch(2);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to second epoch ts
 
         vm.startPrank(address(user));
 
@@ -699,6 +720,7 @@ contract BribeInitiativeAllocateTest is Test {
         vm.stopPrank();
 
         governance.setEpoch(1);
+        vm.warp(governance.EPOCH_DURATION()); // warp to end of first epoch
 
         vm.startPrank(address(governance));
 
@@ -799,6 +821,7 @@ contract BribeInitiativeAllocateTest is Test {
         }
 
         governance.setEpoch(2);
+        vm.warp(block.timestamp + governance.EPOCH_DURATION()); // warp to second epoch ts
 
         vm.startPrank(address(user));
 

test/mocks/MockGovernance.sol

@@ -4,6 +4,9 @@ pragma solidity ^0.8.24;
 contract MockGovernance {
     uint16 private __epoch;
 
+    uint32 constant public EPOCH_START = 0;
+    uint32 constant public EPOCH_DURATION = 7 days;
+
     function claimForInitiative(address) external pure returns (uint256) {
         return 1000e18;
     }