diff --git a/src/Governance.sol b/src/Governance.sol index 56f6cbc9..77d68e47 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -613,9 +613,8 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance (InitiativeStatus status, , ) = getInitiativeState(initiative, votesSnapshot_, votesForInitiativeSnapshot_, initiativeState); if(deltaLQTYVotes > 0 || deltaLQTYVetos > 0) { - /// @audit INVARIANT: `check_unregisterable_consistecy`  - // FSM CHECK, note that the original version allowed voting on `Unregisterable` Initiatives - Prob should fix - require(status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE || status == InitiativeStatus.CLAIMED || status == InitiativeStatus.UNREGISTERABLE, "Governance: active-vote-fsm"); + /// @audit FSM CHECK, note that the original version allowed voting on `Unregisterable` Initiatives | This fixes it + require(status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE || status == InitiativeStatus.CLAIMED, "Governance: active-vote-fsm"); } if(status == InitiativeStatus.DISABLED) { diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 22d98f2b..a4f711b4 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -551,7 +551,7 @@ contract GovernanceTest is Test { lqty.approve(address(userProxy), 1e18); governance.depositLQTY(1e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); // should revert if `_initiative` is zero vm.expectRevert("Governance: zero-address"); @@ -569,6 +569,7 @@ contract GovernanceTest is Test { } // TODO: Broken: Fix it by simplifying most likely + // forge test --match-test test_unregisterInitiative -vv function test_unregisterInitiative() public { vm.startPrank(user); @@ -595,7 +596,7 @@ contract GovernanceTest is Test { lusd.approve(address(governance), 1e18); lqty.approve(address(userProxy), 1e18); governance.depositLQTY(1e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); // should revert if the initiative isn't registered vm.expectRevert("Governance: initiative-not-registered"); @@ -609,13 +610,13 @@ contract GovernanceTest is Test { vm.expectRevert("Governance: initiative-in-warm-up"); /// @audit should fail due to not waiting enough time governance.unregisterInitiative(baseInitiative3); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); // should revert if the initiative is still active or the vetos don't meet the threshold /// @audit TO REVIEW, this never got any votes, so it seems correct to remove // No votes = can be kicked - // vm.expectRevert("Governance: cannot-unregister-initiative"); - // governance.unregisterInitiative(baseInitiative3); + vm.expectRevert("Governance: cannot-unregister-initiative"); + governance.unregisterInitiative(baseInitiative3); snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch() - 1); vm.store( @@ -627,24 +628,7 @@ contract GovernanceTest is Test { assertEq(votes, 1e18); assertEq(forEpoch, governance.epoch() - 1); - IGovernance.InitiativeVoteSnapshot memory initiativeSnapshot = - IGovernance.InitiativeVoteSnapshot(0, governance.epoch() - 1, 0, 0); - vm.store( - address(governance), - keccak256(abi.encode(baseInitiative3, uint256(3))), - bytes32( - abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), - uint16(initiativeSnapshot.forEpoch), - uint224(initiativeSnapshot.votes) - ) - ) - ); - (uint224 votes_, uint16 forEpoch_, uint16 lastCountedEpoch, ) = - governance.votesForInitiativeSnapshot(baseInitiative3); - assertEq(votes_, 0); - assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, 0); + vm.warp(block.timestamp + governance.EPOCH_DURATION() * 3); // 3 more epochs governance.unregisterInitiative(baseInitiative3); @@ -1015,7 +999,7 @@ contract GovernanceTest is Test { vm.expectRevert("Governance: active-vote-fsm"); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); (allocatedLQTY,) = governance.userStates(user); @@ -1033,7 +1017,7 @@ contract GovernanceTest is Test { assertEq(vetoLQTY, 0); // should update the average staking timestamp for the initiative based on the average staking timestamp of the user's // voting and vetoing LQTY - assertEq(averageStakingTimestampVoteLQTY, block.timestamp - 365 days); + assertEq(averageStakingTimestampVoteLQTY, block.timestamp - governance.EPOCH_DURATION()); assertEq(averageStakingTimestampVoteLQTY, averageStakingTimestampUser); assertEq(averageStakingTimestampVetoLQTY, 0); // should remove or add the initiatives voting LQTY from the counter @@ -1057,7 +1041,7 @@ contract GovernanceTest is Test { vm.stopPrank(); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.startPrank(user2); @@ -1086,7 +1070,7 @@ contract GovernanceTest is Test { governance.initiativeStates(baseInitiative1); assertEq(voteLQTY, 2e18); assertEq(vetoLQTY, 0); - assertEq(averageStakingTimestampVoteLQTY, block.timestamp - 365 days); + assertEq(averageStakingTimestampVoteLQTY, block.timestamp - governance.EPOCH_DURATION()); assertGt(averageStakingTimestampVoteLQTY, averageStakingTimestampUser); assertEq(averageStakingTimestampVetoLQTY, 0); @@ -1140,7 +1124,7 @@ contract GovernanceTest is Test { vm.expectRevert("Governance: active-vote-fsm"); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); (allocatedLQTY,) = governance.userStates(user); @@ -1158,7 +1142,7 @@ contract GovernanceTest is Test { assertEq(vetoLQTY, 0); // should update the average staking timestamp for the initiative based on the average staking timestamp of the user's // voting and vetoing LQTY - assertEq(averageStakingTimestampVoteLQTY, block.timestamp - 365 days); + assertEq(averageStakingTimestampVoteLQTY, block.timestamp - governance.EPOCH_DURATION(), "TS"); assertEq(averageStakingTimestampVoteLQTY, averageStakingTimestampUser); assertEq(averageStakingTimestampVetoLQTY, 0); // should remove or add the initiatives voting LQTY from the counter @@ -1182,7 +1166,7 @@ contract GovernanceTest is Test { vm.stopPrank(); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.startPrank(user2); @@ -1211,7 +1195,7 @@ contract GovernanceTest is Test { governance.initiativeStates(baseInitiative1); assertEq(voteLQTY, 2e18); assertEq(vetoLQTY, 0); - assertEq(averageStakingTimestampVoteLQTY, block.timestamp - 365 days); + assertEq(averageStakingTimestampVoteLQTY, block.timestamp - governance.EPOCH_DURATION(), "TS 2"); assertGt(averageStakingTimestampVoteLQTY, averageStakingTimestampUser); assertEq(averageStakingTimestampVetoLQTY, 0); @@ -1256,7 +1240,7 @@ contract GovernanceTest is Test { deltaLQTYVotes[1] = 1e18; int88[] memory deltaLQTYVetos = new int88[](2); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); @@ -1297,7 +1281,7 @@ contract GovernanceTest is Test { deltaLQTYVotes[0] = int88(uint88(_deltaLQTYVotes)); int88[] memory deltaLQTYVetos = new int88[](1); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); @@ -1321,7 +1305,7 @@ contract GovernanceTest is Test { int88[] memory deltaLQTYVetos = new int88[](1); deltaLQTYVetos[0] = int88(uint88(_deltaLQTYVetos)); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); /// @audit needs overflow tests!! @@ -1338,7 +1322,7 @@ contract GovernanceTest is Test { lqty.approve(address(userProxy), 1000e18); governance.depositLQTY(1000e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.stopPrank(); @@ -1425,7 +1409,7 @@ contract GovernanceTest is Test { lqty.approve(address(userProxy), 1000e18); governance.depositLQTY(1000e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.stopPrank(); @@ -1492,7 +1476,7 @@ contract GovernanceTest is Test { function test_multicall() public { vm.startPrank(user); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); uint88 lqtyAmount = 1000e18; uint256 lqtyBalance = lqty.balanceOf(user); @@ -1560,13 +1544,13 @@ contract GovernanceTest is Test { lqty.approve(address(userProxy), 1e18); governance.depositLQTY(1e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); governance.registerInitiative(address(mockInitiative)); uint16 atEpoch = governance.registeredInitiatives(address(mockInitiative)); assertEq(atEpoch, governance.epoch()); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); address[] memory initiatives = new address[](1); initiatives[0] = address(mockInitiative); @@ -1645,7 +1629,7 @@ contract GovernanceTest is Test { deltaLQTYVetos[0] = 0; deltaLQTYVetos[1] = 0; - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.expectRevert("Governance: insufficient-or-allocated-lqty"); governance.allocateLQTY(initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos); diff --git a/test/GovernanceAttacks.t.sol b/test/GovernanceAttacks.t.sol index 287cb28b..a9e2bfa3 100644 --- a/test/GovernanceAttacks.t.sol +++ b/test/GovernanceAttacks.t.sol @@ -81,8 +81,7 @@ contract GovernanceTest is Test { // All calls should never revert due to malicious initiative function test_all_revert_attacks_hardcoded() public { uint256 zeroSnapshot = vm.snapshot(); - uint256 timeIncrease = 86400 * 30; - vm.warp(block.timestamp + timeIncrease); + vm.warp(block.timestamp + governance.EPOCH_DURATION()); vm.startPrank(user); @@ -97,7 +96,6 @@ contract GovernanceTest is Test { assertEq(allocatedLQTY, 0); // first deposit should have an averageStakingTimestamp if block.timestamp assertEq(averageStakingTimestamp, block.timestamp); - vm.warp(block.timestamp + timeIncrease); vm.stopPrank(); diff --git a/test/UserProxy.t.sol b/test/UserProxy.t.sol index 789abffa..17124d52 100644 --- a/test/UserProxy.t.sol +++ b/test/UserProxy.t.sol @@ -115,7 +115,7 @@ contract UserProxyTest is Test { assertEq(lusdAmount, 0); assertEq(ethAmount, 0); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + 7 days); uint256 ethBalance = uint256(vm.load(stakingV1, bytes32(uint256(3)))); vm.store(stakingV1, bytes32(uint256(3)), bytes32(abi.encodePacked(ethBalance + 1e18))); diff --git a/test/recon/CryticToFoundry.sol b/test/recon/CryticToFoundry.sol index 1b61290b..8b2491fe 100644 --- a/test/recon/CryticToFoundry.sol +++ b/test/recon/CryticToFoundry.sol @@ -36,41 +36,6 @@ contract CryticToFoundry is Test, TargetFunctions, FoundryAsserts { property_BI04(); } - // forge test --match-test test_property_BI04_0 -vv - function test_property_BI04_0() public { - - governance_depositLQTY(2); - - vm.roll(block.number + 1); - vm.warp(block.timestamp + 794178); - check_skip_consistecy(0); - - IBribeInitiative initiative = IBribeInitiative(_getDeployedInitiative(0)); - - (uint88 totalLQTYAllocatedAtEpochPrev, ) = initiative.totalLQTYAllocatedByEpoch(governance.epoch()); - vm.roll(block.number + 1); - vm.warp(block.timestamp + 1022610); - - governance_allocateLQTY_clamped_single_initiative(0,1,0); - - (uint88 totalLQTYAllocatedAtEpoch, ) = initiative.totalLQTYAllocatedByEpoch(governance.epoch()); - ( - uint88 voteLQTY, - , - , - , - - ) = governance.initiativeStates(_getDeployedInitiative(0)); - - check_unregisterable_consistecy(0); - - console.log("totalLQTYAllocatedAtEpochPrev", totalLQTYAllocatedAtEpochPrev); - console.log("totalLQTYAllocatedAtEpoch", totalLQTYAllocatedAtEpoch); - console.log("voteLQTY", voteLQTY); - - property_BI04(); - } - // forge test --match-test test_property_resetting_never_reverts_0 -vv function test_property_resetting_never_reverts_0() public { diff --git a/test/recon/properties/GovernanceProperties.sol b/test/recon/properties/GovernanceProperties.sol index 458314ff..663200d4 100644 --- a/test/recon/properties/GovernanceProperties.sol +++ b/test/recon/properties/GovernanceProperties.sol @@ -243,6 +243,10 @@ abstract contract GovernanceProperties is BeforeAfter { } + /// NOTE: This property can break in some specific combinations of: + /// Becomes SKIP due to high treshold + /// threshold is lowered + /// Initiative becomes claimable function check_skip_consistecy(uint8 initiativeIndex) public { // If a initiative has no votes // In the next epoch it can either be SKIP or UNREGISTERABLE @@ -256,8 +260,12 @@ abstract contract GovernanceProperties is BeforeAfter { } } - // TOFIX: The property breaks because you can vote on a UNREGISTERABLE - // Hence it can become Claimable next week + + /// NOTE: This property can break in some specific combinations of: + /// Becomes unregisterable due to high treshold + /// Is not unregistered + /// threshold is lowered + /// Initiative becomes claimable function check_unregisterable_consistecy(uint8 initiativeIndex) public { // If a initiative has no votes and is UNREGISTERABLE // In the next epoch it will remain UNREGISTERABLE @@ -267,7 +275,7 @@ abstract contract GovernanceProperties is BeforeAfter { if(status == Governance.InitiativeStatus.UNREGISTERABLE) { vm.warp(block.timestamp + governance.EPOCH_DURATION()); (Governance.InitiativeStatus newStatus,,) = governance.getInitiativeState(initiative); - t(uint256(status) == uint256(newStatus) || uint256(newStatus) == uint256(Governance.InitiativeStatus.CLAIMABLE), "UNREGISTERABLE must remain UNREGISTERABLE unless voted on but can become CLAIMABLE due to relaxed checks in allocateLQTY"); + t(uint256(status) == uint256(newStatus), "UNREGISTERABLE must remain UNREGISTERABLE"); } }