Merge branch 'exp-reset-all' into fix-audit-and-invariants

ToFix.MD

@@ -0,0 +1,29 @@
+- 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

src/CurveV2GaugeRewards.sol

@@ -26,6 +26,7 @@ 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;
 
@@ -37,7 +38,7 @@ contract CurveV2GaugeRewards is BribeInitiative {
 
         remainder = 0;
 
-        bold.approve(address(gauge), total);
+        bold.approve(address(gauge), total); 
         gauge.deposit_reward_token(address(bold), total, duration);
 
         emit DepositIntoGauge(total);

src/Governance.sol

@@ -157,7 +157,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
                                 STAKING
     //////////////////////////////////////////////////////////////*/
 
-    function _deposit(uint88 _lqtyAmount) private returns (UserProxy) {
+    function _updateUserStakes(uint88 _lqtyAmount) private returns (UserProxy) {
         require(_lqtyAmount > 0, "Governance: zero-lqty-amount");
 
         address userProxyAddress = deriveUserProxyAddress(msg.sender);
@@ -170,8 +170,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
         uint88 lqtyStaked = uint88(stakingV1.stakes(userProxyAddress));
 
-        // update the average staked timestamp for LQTY staked by the user
         UserState memory userState = userStates[msg.sender];
+        // Assert that we have resetted here
+        require(userState.allocatedLQTY == 0, "Governance: must-be-zero-allocation");
+
+        // update the average staked timestamp for LQTY staked by the user
+
         userState.averageStakingTimestamp = _calculateAverageTimestamp(
             userState.averageStakingTimestamp, uint32(block.timestamp), lqtyStaked, lqtyStaked + _lqtyAmount
         );
@@ -184,13 +188,13 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
     /// @inheritdoc IGovernance
     function depositLQTY(uint88 _lqtyAmount) external nonReentrant {
-        UserProxy userProxy = _deposit(_lqtyAmount);
+        UserProxy userProxy = _updateUserStakes(_lqtyAmount);
         userProxy.stake(_lqtyAmount, msg.sender);
     }
 
     /// @inheritdoc IGovernance
     function depositLQTYViaPermit(uint88 _lqtyAmount, PermitParams calldata _permitParams) external nonReentrant {
-        UserProxy userProxy = _deposit(_lqtyAmount);
+        UserProxy userProxy = _updateUserStakes(_lqtyAmount);
         userProxy.stakeViaPermit(_lqtyAmount, msg.sender, _permitParams);
     }
 
@@ -204,7 +208,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
         UserState storage userState = userStates[msg.sender];
 
         // check if user has enough unallocated lqty
-        require(_lqtyAmount <= lqtyStaked - userState.allocatedLQTY, "Governance: insufficient-unallocated-lqty");
+        require(userState.allocatedLQTY == 0, "Governance: must-allocate-zero");
 
         (uint256 accruedLUSD, uint256 accruedETH) = userProxy.unstake(_lqtyAmount, msg.sender);
 
@@ -528,7 +532,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
             // Must be below, else we cannot reset"
             // Makes cast safe
-            /// @audit INVARIANT: property_ensure_user_alloc_cannot_dos
+            /// @audit Check INVARIANT: property_ensure_user_alloc_cannot_dos
             assert(alloc.voteLQTY <= uint88(type(int88).max));
             assert(alloc.vetoLQTY <= uint88(type(int88).max));
 
@@ -550,6 +554,15 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
         return cachedData;
     }
 
+    /// @notice Reset the allocations for the initiatives being passed, must pass all initiatives else it will revert
+    ///     NOTE: If you reset at the last day of the epoch, you won't be able to vote again
+    ///         Use `allocateLQTY` to reset and vote
+    function resetAllocations(address[] calldata _initiativesToReset) external nonReentrant {
+        _requireNoDuplicates(_initiativesToReset);
+        _resetInitiatives(_initiativesToReset);
+        require(userStates[msg.sender].allocatedLQTY == 0, "must be a reset");
+    }
+
     /// @inheritdoc IGovernance
     function allocateLQTY(
         address[] calldata _initiativesToReset,
@@ -641,7 +654,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
                 getInitiativeState(initiative, votesSnapshot_, votesForInitiativeSnapshot_, initiativeState);
 
             if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) {
-                /// @audit FSM CHECK, note that the original version allowed voting on `Unregisterable` Initiatives | This fixes it
+                /// @audit You cannot vote on `unregisterable` but a vote may have been there
                 require(
                     status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE
                         || status == InitiativeStatus.CLAIMED,
@@ -668,13 +681,13 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
             // update the average staking timestamp for the initiative based on the user's average staking timestamp
             initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp(
                 initiativeState.averageStakingTimestampVoteLQTY,
-                userState.averageStakingTimestamp, /// @audit This is wrong, we need to remove it from the allocation
+                userState.averageStakingTimestamp, /// @audit This is wrong unless we enforce a reset on deposit and withdrawal
                 initiativeState.voteLQTY,
                 add(initiativeState.voteLQTY, deltaLQTYVotes)
             );
             initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp(
                 initiativeState.averageStakingTimestampVetoLQTY,
-                userState.averageStakingTimestamp, /// @audit This is wrong, we need to remove it from the allocation
+                userState.averageStakingTimestamp, /// @audit This is wrong unless we enforce a reset on deposit and withdrawal
                 initiativeState.vetoLQTY,
                 add(initiativeState.vetoLQTY, deltaLQTYVetos)
             );

test/BribeInitiative.t.sol

@@ -574,58 +574,8 @@ contract BribeInitiativeTest is Test {
         assertEq(bribeTokenAmount, 0, "vetoer receives bribe amount");
     }
 
-    // TODO: check favorability of splitting allocation between different initiative/epochs
-    // @audit doesn't seem like it makes it more favorable because user still withdraws full bribe amount
-    // forge test --match-test test_splitting_allocation -vv
-    function test_splitting_allocation() public {
-        // =========== epoch 1 ==================
-        // user stakes half in epoch 1
-        int88 lqtyAmount = 2e18;
-        _stakeLQTY(user1, uint88(lqtyAmount / 2));
-
-        // =========== epoch 2 ==================
-        vm.warp(block.timestamp + EPOCH_DURATION);
-        assertEq(2, governance.epoch(), "not in epoch 2");
-
-        // lusdHolder deposits lqty and lusd bribes claimable in epoch 4
-        _depositBribe(1e18, 1e18, governance.epoch() + 1);
-        uint16 epochToClaimFor = governance.epoch() + 1;
-
-        // user votes on bribeInitiative with half
-        _allocateLQTY(user1, lqtyAmount / 2, 0);
-        (, uint32 averageStakingTimestamp1) = governance.userStates(user1);
-
-        uint16 epochDepositedHalf = governance.epoch();
-
-        // =========== epoch 2 (end of cutoff) ==================
-        vm.warp(block.timestamp + EPOCH_DURATION - EPOCH_VOTING_CUTOFF);
-        assertEq(2, governance.epoch(), "not in epoch 2");
 
-        // user stakes other half
-        _stakeLQTY(user1, uint88(lqtyAmount / 2));
-        // user votes on bribeInitiative with other half
-        _allocateLQTY(user1, lqtyAmount / 2, 0);
-
-        uint16 epochDepositedRest = governance.epoch();
-        (, uint32 averageStakingTimestamp2) = governance.userStates(user1);
-        assertTrue(
-            averageStakingTimestamp1 != averageStakingTimestamp2, "averageStakingTimestamp1 == averageStakingTimestamp2"
-        );
-
-        assertEq(epochDepositedHalf, epochDepositedRest, "We are in the same epoch");
-
-        // =========== epoch 4 ==================
-        vm.warp(block.timestamp + (EPOCH_DURATION * 2));
-        assertEq(4, governance.epoch(), "not in epoch 4");
-
-        // user should receive bribe from their allocated stake
-        (uint256 boldAmount, uint256 bribeTokenAmount) =
-            _claimBribe(user1, epochToClaimFor, epochDepositedRest, epochDepositedRest);
-        assertEq(boldAmount, 1e18, "boldAmount");
-        assertEq(bribeTokenAmount, 1e18, "bribeTokenAmount");
-
-        // With non spliting the amount would be 1e18, so this is a bug due to how allocations work
-    }
+
 
     // checks that user can receive bribes for an epoch in which they were allocated even if they're no longer allocated
     function test_decrement_after_claimBribes() public {

test/Governance.t.sol

@@ -170,6 +170,7 @@ contract GovernanceTest is Test {
         );
     }
 
+    // forge test --match-test test_depositLQTY_withdrawLQTY -vv
     function test_depositLQTY_withdrawLQTY() public {
         uint256 timeIncrease = 86400 * 30;
         vm.warp(block.timestamp + timeIncrease);
@@ -220,9 +221,6 @@ contract GovernanceTest is Test {
 
         vm.startPrank(user);
 
-        vm.expectRevert("Governance: insufficient-unallocated-lqty");
-        governance.withdrawLQTY(type(uint88).max);
-
         governance.withdrawLQTY(1e18);
         assertEq(UserProxy(payable(userProxy)).staked(), 1e18);
         (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user);
@@ -1119,7 +1117,7 @@ contract GovernanceTest is Test {
         assertEq(averageStakingTimestampVetoLQTY, 0);
 
         // should revert if the user doesn't have enough unallocated LQTY available
-        vm.expectRevert("Governance: insufficient-unallocated-lqty");
+        vm.expectRevert("Governance: must-allocate-zero");
         governance.withdrawLQTY(1e18);
 
         vm.warp(block.timestamp + EPOCH_DURATION - governance.secondsWithinEpoch() - 1);
@@ -1243,7 +1241,7 @@ contract GovernanceTest is Test {
         assertEq(averageStakingTimestampVetoLQTY, 0);
 
         // should revert if the user doesn't have enough unallocated LQTY available
-        vm.expectRevert("Governance: insufficient-unallocated-lqty");
+        vm.expectRevert("Governance: must-allocate-zero");
         governance.withdrawLQTY(1e18);
 
         vm.warp(block.timestamp + EPOCH_DURATION - governance.secondsWithinEpoch() - 1);

test/VotingPower.t.sol

@@ -424,77 +424,6 @@ contract VotingPowerTest is Test {
         _allocate(address(baseInitiative1), 0, lqtyAmount);
     }
 
-    //// Compare the relative power per epoch
-    /// As in, one epoch should reliably increase the power by X amt
-    // forge test --match-test test_allocation_avg_ts_mismatch -vv
-    function test_allocation_avg_ts_mismatch() public {
-        uint256 snapshot0 = vm.snapshot();
-
-        uint256 snapshotBefore = vm.snapshot();
-
-        vm.startPrank(user);
-        // =========== epoch 1 ==================
-        // 1. user stakes lqty
-        int88 lqtyAmount = 2e18;
-        _stakeLQTY(user, uint88(lqtyAmount / 2));
-
-        // user allocates to baseInitiative1
-        _allocate(address(baseInitiative1), lqtyAmount / 2, 0); // 50% to it
-        (, uint32 averageStakingTimestamp1) = governance.userStates(user);
-
-        // =========== epoch 2 (start) ==================
-        // 2. user allocates in epoch 2
-        vm.warp(block.timestamp + EPOCH_DURATION); // warp to second epoch
-
-        // Remainer
-        _stakeLQTY(user, uint88(lqtyAmount / 2));
-        _allocate(address(baseInitiative2), lqtyAmount / 2, 0); // 50% to it
-
-        (, uint32 averageStakingTimestamp2) = governance.userStates(user);
-
-        assertGt(averageStakingTimestamp2, averageStakingTimestamp1, "Time increase");
-
-        // Get TS for "exploit"
-        uint256 avgTs1 = _getAverageTS(baseInitiative1);
-        uint256 avgTs2 = _getAverageTS(baseInitiative2);
-        assertGt(avgTs2, avgTs1, "TS in initiative is increased");
-
-        // Check if Resetting will fix the issue
-
-        _allocate(address(baseInitiative1), 0, 0);
-        _allocate(address(baseInitiative2), 0, 0);
-
-        _allocate(address(baseInitiative1), 0, 0);
-        _allocate(address(baseInitiative2), 0, 0);
-
-        uint256 avgTs_reset_1 = _getAverageTS(baseInitiative1);
-        uint256 avgTs_reset_2 = _getAverageTS(baseInitiative2);
-
-        // Intuition, Delta time * LQTY = POWER
-        vm.revertTo(snapshotBefore);
-
-        // Compare against
-        // Deposit 1 on epoch 1
-        // Deposit 2 on epoch 2
-        // Vote on epoch 2 exclusively
-        _stakeLQTY(user, uint88(lqtyAmount / 2));
-
-        vm.warp(block.timestamp + EPOCH_DURATION); // warp to second epoch
-        _stakeLQTY(user, uint88(lqtyAmount / 2));
-        _allocate(address(baseInitiative2), lqtyAmount / 2, 0); // 50% to it
-        _allocate(address(baseInitiative1), lqtyAmount / 2, 0); // 50% to it
-
-        uint256 avgTs1_diff = _getAverageTS(baseInitiative1);
-        uint256 avgTs2_diff = _getAverageTS(baseInitiative2);
-        // assertEq(avgTs2_diff, avgTs1_diff, "TS in initiative is increased");
-        assertGt(avgTs1_diff, avgTs2_diff, "TS in initiative is increased");
-
-        assertLt(avgTs2_diff, avgTs2, "Ts2 is same");
-        assertGt(avgTs1_diff, avgTs1, "Ts1 lost the power");
-
-        assertLt(avgTs_reset_1, avgTs1_diff, "Same as diff means it does reset");
-        assertEq(avgTs_reset_2, avgTs2_diff, "Same as diff means it does reset");
-    }
 
     // Check if Flashloan can be used to cause issues?
     // A flashloan would cause issues in the measure in which it breaks any specific property
@@ -533,4 +462,13 @@ contract VotingPowerTest is Test {
 
         governance.allocateLQTY(initiativesToReset, initiatives, deltaLQTYVotes, deltaLQTYVetos);
     }
+
+    function _reset() internal {
+        address[] memory initiativesToReset = new address[](3);
+        initiativesToReset[0] = baseInitiative1;
+        initiativesToReset[1] = baseInitiative2;
+        initiativesToReset[2] = baseInitiative3;
+
+        governance.resetAllocations(initiativesToReset);
+    }
 }

test/recon/Properties.sol

@@ -4,5 +4,6 @@ pragma solidity ^0.8.0;
 import {BeforeAfter} from "./BeforeAfter.sol";
 import {GovernanceProperties} from "./properties/GovernanceProperties.sol";
 import {BribeInitiativeProperties} from "./properties/BribeInitiativeProperties.sol";
+import {SynchProperties} from "./properties/SynchProperties.sol";
 
-abstract contract Properties is GovernanceProperties, BribeInitiativeProperties {}
+abstract contract Properties is GovernanceProperties, BribeInitiativeProperties, SynchProperties {}