From 9ad09cfb1010ccf7216bc3aaf37807126a26c4c1 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 11:30:18 +0100 Subject: [PATCH 01/11] chore: rename _deposit --- ToFix.MD | 14 ++++++++++++++ src/Governance.sol | 13 ++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 ToFix.MD diff --git a/ToFix.MD b/ToFix.MD new file mode 100644 index 00000000..39c42359 --- /dev/null +++ b/ToFix.MD @@ -0,0 +1,14 @@ +- Add properties check to ensure that the math is sound + +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 + +- 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 \ No newline at end of file diff --git a/src/Governance.sol b/src/Governance.sol index 85eda250..183d24ef 100644 --- a/src/Governance.sol +++ b/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); @@ -184,13 +184,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); } @@ -550,6 +550,13 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance return cachedData; } + function resetAllocations(address[] calldata _initiativesToReset) external nonReentrant { + _requireNoDuplicates(_initiativesToReset); + _resetInitiatives(_initiativesToReset); + + assert(userState.allocatedLQTY == 0); + } + /// @inheritdoc IGovernance function allocateLQTY( address[] calldata _initiativesToReset, From 8e6a64592b03fca5e52c1fb1f335eb74c401a407 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 12:25:59 +0100 Subject: [PATCH 02/11] chore: cleanup --- test/recon/targets/GovernanceTargets.sol | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/recon/targets/GovernanceTargets.sol b/test/recon/targets/GovernanceTargets.sol index 4b81fac5..2a0644ff 100644 --- a/test/recon/targets/GovernanceTargets.sol +++ b/test/recon/targets/GovernanceTargets.sol @@ -61,11 +61,6 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { // Whereas for the user it could | TODO eq(after_global_allocatedLQTY, b4_global_allocatedLQTY, "Same alloc"); } - - - // Math should be: - // Result of reset - // Result of vote } // Resetting never fails and always resets From 0266514a18e7b3e9d252089a14742e86ff14cf8d Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 12:26:13 +0100 Subject: [PATCH 03/11] feat: stateful test for reset soundness --- .../recon/properties/GovernanceProperties.sol | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/test/recon/properties/GovernanceProperties.sol b/test/recon/properties/GovernanceProperties.sol index 71cbbd1d..d9c42945 100644 --- a/test/recon/properties/GovernanceProperties.sol +++ b/test/recon/properties/GovernanceProperties.sol @@ -6,6 +6,7 @@ import {Governance} from "src/Governance.sol"; import {IGovernance} from "src/interfaces/IGovernance.sol"; import {MockStakingV1} from "test/mocks/MockStakingV1.sol"; import {vm} from "@chimera/Hevm.sol"; +import {IUserProxy} from "src/interfaces/IUserProxy.sol"; abstract contract GovernanceProperties is BeforeAfter { /// A Initiative cannot change in status @@ -312,4 +313,86 @@ abstract contract GovernanceProperties is BeforeAfter { vetos += allocVetos; } } + + function property_alloc_deposit_reset_is_idempotent( + uint8 initiativesIndex, + uint96 deltaLQTYVotes, + uint96 deltaLQTYVetos, + uint88 lqtyAmount + ) public withChecks { + + address targetInitiative = _getDeployedInitiative(initiativesIndex); + + // 0. Reset first to ensure we start fresh, else the totals can be out of whack + // TODO: prob unnecessary + // Cause we always reset anyway + { + int88[] memory zeroes = new int88[](deployedInitiatives.length); + + governance.allocateLQTY(deployedInitiatives, deployedInitiatives, zeroes, zeroes); + } + + // GET state and initiative data before allocation + ( + uint88 totalCountedLQTY, + uint32 user_countedVoteLQTYAverageTimestamp + ) = governance.globalState(); + ( + uint88 voteLQTY, + uint88 vetoLQTY, + uint32 averageStakingTimestampVoteLQTY, + uint32 averageStakingTimestampVetoLQTY, + + ) = governance.initiativeStates(targetInitiative); + + // Allocate + { + uint96 stakedAmount = IUserProxy(governance.deriveUserProxyAddress(user)).staked(); + + address[] memory initiatives = new address[](1); + initiatives[0] = targetInitiative; + int88[] memory deltaLQTYVotesArray = new int88[](1); + deltaLQTYVotesArray[0] = int88(uint88(deltaLQTYVotes % stakedAmount)); + int88[] memory deltaLQTYVetosArray = new int88[](1); + deltaLQTYVetosArray[0] = int88(uint88(deltaLQTYVetos % stakedAmount)); + + governance.allocateLQTY(deployedInitiatives, initiatives, deltaLQTYVotesArray, deltaLQTYVetosArray); + } + + + // Deposit (Changes total LQTY an hopefully also changes ts) + { + lqtyAmount = uint88(lqtyAmount % lqty.balanceOf(user)); + governance.depositLQTY(lqtyAmount); + } + + // REMOVE STUFF to remove the user data + { + int88[] memory zeroes = new int88[](deployedInitiatives.length); + governance.allocateLQTY(deployedInitiatives, deployedInitiatives, zeroes, zeroes); + } + + // Check total allocation and initiative allocation + { + ( + uint88 after_totalCountedLQTY, + uint32 after_user_countedVoteLQTYAverageTimestamp + ) = governance.globalState(); + ( + uint88 after_voteLQTY, + uint88 after_vetoLQTY, + uint32 after_averageStakingTimestampVoteLQTY, + uint32 after_averageStakingTimestampVetoLQTY, + + ) = governance.initiativeStates(targetInitiative); + + eq(voteLQTY, after_voteLQTY, "Same vote"); + eq(vetoLQTY, after_vetoLQTY, "Same veto"); + eq(averageStakingTimestampVoteLQTY, after_averageStakingTimestampVoteLQTY, "Same ts vote"); + eq(averageStakingTimestampVetoLQTY, after_averageStakingTimestampVetoLQTY, "Same ts veto"); + + eq(totalCountedLQTY, after_totalCountedLQTY, "Same total LQTY"); + eq(user_countedVoteLQTYAverageTimestamp, after_user_countedVoteLQTYAverageTimestamp, "Same total ts"); + } + } } From aff1befc4108a97ffb09d5572a1bd9cb797e13fb Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 12:26:23 +0100 Subject: [PATCH 04/11] chore: undo yolo reset change --- ToFix.MD | 17 ++++++++++++++++- src/Governance.sol | 2 -- test/Governance.t.sol | 6 +++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/ToFix.MD b/ToFix.MD index 39c42359..99cac390 100644 --- a/ToFix.MD +++ b/ToFix.MD @@ -1,13 +1,28 @@ -- Add properties check to ensure that the math is sound +- 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 diff --git a/src/Governance.sol b/src/Governance.sol index 183d24ef..64f39587 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -553,8 +553,6 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance function resetAllocations(address[] calldata _initiativesToReset) external nonReentrant { _requireNoDuplicates(_initiativesToReset); _resetInitiatives(_initiativesToReset); - - assert(userState.allocatedLQTY == 0); } /// @inheritdoc IGovernance diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 957873b2..6b3fc401 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -220,7 +220,7 @@ contract GovernanceTest is Test { vm.startPrank(user); - vm.expectRevert("Governance: insufficient-unallocated-lqty"); + vm.expectRevert("Governance: must-allocate-zero"); governance.withdrawLQTY(type(uint88).max); governance.withdrawLQTY(1e18); @@ -1119,7 +1119,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 +1243,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); From 4a73edcc3a1c51f04ad6e0cd7cfa3e8a7071f8c4 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 13:34:52 +0100 Subject: [PATCH 05/11] feat: can be reprod manually --- test/recon/CryticToFoundry.sol | 24 +++++++++++ .../recon/properties/GovernanceProperties.sol | 5 +++ test/recon/targets/GovernanceTargets.sol | 40 ++++++++++++++++++- 3 files changed, 67 insertions(+), 2 deletions(-) diff --git a/test/recon/CryticToFoundry.sol b/test/recon/CryticToFoundry.sol index ebf54136..26779784 100644 --- a/test/recon/CryticToFoundry.sol +++ b/test/recon/CryticToFoundry.sol @@ -79,4 +79,28 @@ contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts { property_BI04(); } + + // forge test --match-test test_manual_check -vv + function test_manual_check() public { + governance_depositLQTY(1e18); + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 123); + governance_depositLQTY_2(1e18 + 1); + + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 654326); + governance_allocateLQTY_clamped_single_initiative(0, 99999999999999999, 0); + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 654326); + governance_allocateLQTY_clamped_single_initiative_2nd_user(0, 99999999999999999, 0); + console.log("user2 is done"); + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 654326); + property_alloc_deposit_reset_is_idempotent(0, 99999999999999999, 0, 99999999999999999); + console.log("user 1 do undo is done"); + } } diff --git a/test/recon/properties/GovernanceProperties.sol b/test/recon/properties/GovernanceProperties.sol index d9c42945..45856f88 100644 --- a/test/recon/properties/GovernanceProperties.sol +++ b/test/recon/properties/GovernanceProperties.sol @@ -362,8 +362,13 @@ abstract contract GovernanceProperties is BeforeAfter { // Deposit (Changes total LQTY an hopefully also changes ts) { + (, uint32 averageStakingTimestamp1) = governance.userStates(user); + lqtyAmount = uint88(lqtyAmount % lqty.balanceOf(user)); governance.depositLQTY(lqtyAmount); + (, uint32 averageStakingTimestamp2) = governance.userStates(user); + + require(averageStakingTimestamp2 > averageStakingTimestamp1, "Must have changed"); } // REMOVE STUFF to remove the user data diff --git a/test/recon/targets/GovernanceTargets.sol b/test/recon/targets/GovernanceTargets.sol index 2a0644ff..94e70424 100644 --- a/test/recon/targets/GovernanceTargets.sol +++ b/test/recon/targets/GovernanceTargets.sol @@ -23,6 +23,7 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { uint96 deltaLQTYVotes, uint96 deltaLQTYVetos ) public withChecks { + uint16 currentEpoch = governance.epoch(); uint96 stakedAmount = IUserProxy(governance.deriveUserProxyAddress(user)).staked(); // clamp using the user's staked balance @@ -41,7 +42,6 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { (Governance.InitiativeStatus status, ,) = governance.getInitiativeState(initiatives[0]); - governance.allocateLQTY(deployedInitiatives, initiatives, deltaLQTYVotesArray, deltaLQTYVetosArray); // The test here should be: @@ -63,6 +63,28 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { } } + function governance_allocateLQTY_clamped_single_initiative_2nd_user( + uint8 initiativesIndex, + uint96 deltaLQTYVotes, + uint96 deltaLQTYVetos + ) public withChecks { + + uint16 currentEpoch = governance.epoch(); + uint96 stakedAmount = IUserProxy(governance.deriveUserProxyAddress(user2)).staked(); // clamp using the user's staked balance + + address[] memory initiatives = new address[](1); + initiatives[0] = _getDeployedInitiative(initiativesIndex); + int88[] memory deltaLQTYVotesArray = new int88[](1); + deltaLQTYVotesArray[0] = int88(uint88(deltaLQTYVotes % stakedAmount)); + int88[] memory deltaLQTYVetosArray = new int88[](1); + deltaLQTYVetosArray[0] = int88(uint88(deltaLQTYVetos % stakedAmount)); + + require(stakedAmount > 0, "0 stake"); + + vm.prank(user2); + governance.allocateLQTY(deployedInitiatives, initiatives, deltaLQTYVotesArray, deltaLQTYVetosArray); + } + // Resetting never fails and always resets function property_resetting_never_reverts() public withChecks { int88[] memory zeroes = new int88[](deployedInitiatives.length); @@ -133,6 +155,20 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { lqtyAmount = uint88(lqtyAmount % lqty.balanceOf(user)); governance.depositLQTY(lqtyAmount); } + function governance_depositLQTY_2(uint88 lqtyAmount) public withChecks { + // Deploy and approve since we don't do it in constructor + vm.prank(user2); + try governance.deployUserProxy() returns (address proxy) { + vm.prank(user2); + lqty.approve(proxy, type(uint88).max); + } catch { + + } + + lqtyAmount = uint88(lqtyAmount % lqty.balanceOf(user2)); + vm.prank(user2); + governance.depositLQTY(lqtyAmount); + } function governance_depositLQTYViaPermit(uint88 _lqtyAmount) public withChecks { // Get the current block timestamp for the deadline @@ -154,7 +190,7 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { PermitParams memory permitParams = PermitParams({owner: user2, spender: user, value: _lqtyAmount, deadline: deadline, v: v, r: r, s: s}); - + // TODO: BROKEN governance.depositLQTYViaPermit(_lqtyAmount, permitParams); } From 432e88c1f06213c88742b47fdd182d88406142ee Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 14:01:49 +0100 Subject: [PATCH 06/11] chore: repro --- test/recon/CryticToFoundry.sol | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/recon/CryticToFoundry.sol b/test/recon/CryticToFoundry.sol index 26779784..e9c635e7 100644 --- a/test/recon/CryticToFoundry.sol +++ b/test/recon/CryticToFoundry.sol @@ -103,4 +103,24 @@ contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts { property_alloc_deposit_reset_is_idempotent(0, 99999999999999999, 0, 99999999999999999); console.log("user 1 do undo is done"); } + + // forge test --match-test test_property_alloc_deposit_reset_is_idempotent_3 -vv + function test_property_alloc_deposit_reset_is_idempotent_3() public { + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 429952); + governance_depositLQTY_2(2); + + vm.roll(block.number + 1); + vm.warp(block.timestamp + 179265); + + governance_allocateLQTY_clamped_single_initiative_2nd_user(0,1,0); + + governance_depositLQTY(2); + + check_skip_consistecy(0); + + property_alloc_deposit_reset_is_idempotent(0,1,0,1); + + } } From 5a82349a8c47d1e6c30a29e9d7a7842c387ef11d Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 14:52:35 +0100 Subject: [PATCH 07/11] chore: future notes on V2 Gauge --- src/CurveV2GaugeRewards.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/CurveV2GaugeRewards.sol b/src/CurveV2GaugeRewards.sol index 5d9d9824..647edbe8 100644 --- a/src/CurveV2GaugeRewards.sol +++ b/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 { // For small donations queue them into the contract if (amount < duration * 1000) { @@ -36,7 +37,7 @@ contract CurveV2GaugeRewards is BribeInitiative { uint256 total = amount + remainder; remainder = 0; - bold.approve(address(gauge), total); + bold.approve(address(gauge), total); gauge.deposit_reward_token(address(bold), total, duration); emit DepositIntoGauge(total); From 696cfde5fb6ab031e00a4d15a866c7e4fdbebac8 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 15:08:02 +0100 Subject: [PATCH 08/11] chore: make tests compile --- src/Governance.sol | 12 ++++- test/BribeInitiative.t.sol | 52 +--------------------- test/Governance.t.sol | 4 +- test/VotingPower.t.sol | 80 ++++------------------------------ test/recon/CryticToFoundry.sol | 44 ------------------- 5 files changed, 21 insertions(+), 171 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index 64f39587..e3d98e19 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -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 ); @@ -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); @@ -550,9 +554,13 @@ 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 diff --git a/test/BribeInitiative.t.sol b/test/BribeInitiative.t.sol index a922c0e5..322a2a92 100644 --- a/test/BribeInitiative.t.sol +++ b/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 { diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 6b3fc401..2d9221ab 100644 --- a/test/Governance.t.sol +++ b/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: must-allocate-zero"); - governance.withdrawLQTY(type(uint88).max); - governance.withdrawLQTY(1e18); assertEq(UserProxy(payable(userProxy)).staked(), 1e18); (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user); diff --git a/test/VotingPower.t.sol b/test/VotingPower.t.sol index 861ce24a..e27f72ae 100644 --- a/test/VotingPower.t.sol +++ b/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); + } } diff --git a/test/recon/CryticToFoundry.sol b/test/recon/CryticToFoundry.sol index e9c635e7..ebf54136 100644 --- a/test/recon/CryticToFoundry.sol +++ b/test/recon/CryticToFoundry.sol @@ -79,48 +79,4 @@ contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts { property_BI04(); } - - // forge test --match-test test_manual_check -vv - function test_manual_check() public { - governance_depositLQTY(1e18); - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 123); - governance_depositLQTY_2(1e18 + 1); - - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 654326); - governance_allocateLQTY_clamped_single_initiative(0, 99999999999999999, 0); - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 654326); - governance_allocateLQTY_clamped_single_initiative_2nd_user(0, 99999999999999999, 0); - console.log("user2 is done"); - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 654326); - property_alloc_deposit_reset_is_idempotent(0, 99999999999999999, 0, 99999999999999999); - console.log("user 1 do undo is done"); - } - - // forge test --match-test test_property_alloc_deposit_reset_is_idempotent_3 -vv - function test_property_alloc_deposit_reset_is_idempotent_3() public { - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 429952); - governance_depositLQTY_2(2); - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 179265); - - governance_allocateLQTY_clamped_single_initiative_2nd_user(0,1,0); - - governance_depositLQTY(2); - - check_skip_consistecy(0); - - property_alloc_deposit_reset_is_idempotent(0,1,0,1); - - } } From 66a29a09fcae6ab69b1aa01e7001a090eee1d38c Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 15:20:27 +0100 Subject: [PATCH 09/11] feat: aded reset alloc calls --- test/recon/targets/GovernanceTargets.sol | 33 ++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/recon/targets/GovernanceTargets.sol b/test/recon/targets/GovernanceTargets.sol index 94e70424..e65be6f9 100644 --- a/test/recon/targets/GovernanceTargets.sol +++ b/test/recon/targets/GovernanceTargets.sol @@ -85,6 +85,16 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { governance.allocateLQTY(deployedInitiatives, initiatives, deltaLQTYVotesArray, deltaLQTYVetosArray); } + function governance_resetAllocations() public { + governance.resetAllocations(deployedInitiatives); + } + function governance_resetAllocations_user_2() public { + vm.prank(user2); + governance.resetAllocations(deployedInitiatives); + } + + // TODO: if userState.allocatedLQTY != 0 deposit and withdraw must always revert + // Resetting never fails and always resets function property_resetting_never_reverts() public withChecks { int88[] memory zeroes = new int88[](deployedInitiatives.length); @@ -99,6 +109,29 @@ abstract contract GovernanceTargets is BaseTargetFunctions, Properties { eq(user_allocatedLQTY, 0, "User has 0 allocated on a reset"); } + function depositMustFailOnNonZeroAlloc(uint88 lqtyAmount) public withChecks { + (uint88 user_allocatedLQTY,) = governance.userStates(user); + + require(user_allocatedLQTY != 0); + + lqtyAmount = uint88(lqtyAmount % lqty.balanceOf(user)); + try governance.depositLQTY(lqtyAmount) { + t(false, "Deposit Must always revert when user is not reset"); + } catch { + } + } + + function withdrwaMustFailOnNonZeroAcc(uint88 _lqtyAmount) public withChecks { + (uint88 user_allocatedLQTY,) = governance.userStates(user); + + require(user_allocatedLQTY != 0); + + try governance.withdrawLQTY(_lqtyAmount) { + t(false, "Withdraw Must always revert when user is not reset"); + } catch { + } + } + // For every previous epoch go grab ghost values and ensure they match snapshot // For every initiative, make ghost values and ensure they match // For all operations, you also need to add the VESTED AMT? From 5cfeff061a0769492108b948d4c2d80622cf2720 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 15:43:28 +0100 Subject: [PATCH 10/11] feat: new property for reset --- test/recon/Properties.sol | 3 +- test/recon/properties/SynchProperties.sol | 45 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 test/recon/properties/SynchProperties.sol diff --git a/test/recon/Properties.sol b/test/recon/Properties.sol index f9ffc54a..d3d7c6a9 100644 --- a/test/recon/Properties.sol +++ b/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 {} diff --git a/test/recon/properties/SynchProperties.sol b/test/recon/properties/SynchProperties.sol new file mode 100644 index 00000000..7ff77354 --- /dev/null +++ b/test/recon/properties/SynchProperties.sol @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +pragma solidity ^0.8.0; + +import {BeforeAfter} from "../BeforeAfter.sol"; +import {Governance} from "src/Governance.sol"; +import {IGovernance} from "src/interfaces/IGovernance.sol"; +import {IBribeInitiative} from "src/interfaces/IBribeInitiative.sol"; + +abstract contract SynchProperties is BeforeAfter { + // Properties that ensure that the states are synched + + // Go through each initiative + // Go through each user + // Ensure that a non zero vote uses the user latest TS + // This ensures that the math is correct in removal and addition + function property_initiative_ts_matches_user_when_non_zero() public { + // For all strategies + for (uint256 i; i < deployedInitiatives.length; i++) { + for (uint256 j; j < users.length; j++) { + (uint88 votes, , uint16 epoch) = governance.lqtyAllocatedByUserToInitiative(users[j], deployedInitiatives[i]); + + // Grab epoch from initiative + (uint88 lqtyAllocatedByUserAtEpoch, uint32 ts) = + IBribeInitiative(deployedInitiatives[i]).lqtyAllocatedByUserAtEpoch(users[j], epoch); + + // Check that TS matches (only for votes) + eq(lqtyAllocatedByUserAtEpoch, votes, "Votes must match at all times"); + + if(votes != 0) { + // if we're voting and the votes are different from 0 + // then we check user TS + (, uint32 averageStakingTimestamp) = governance.userStates(users[j]); + + eq(averageStakingTimestamp, ts, "Timestamp must be most recent when it's non zero"); + } else { + // NOTE: If votes are zero the TS is passed, but it is not a useful value + // This is left here as a note for the reviewer + } + } + } + + } + + +} \ No newline at end of file From b2972be8b3aec807c4ffcabb461676258e11c6a5 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 29 Oct 2024 16:04:50 +0100 Subject: [PATCH 11/11] chore: comments --- src/Governance.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index e3d98e19..13f0668d 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -532,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)); @@ -654,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, @@ -681,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) );