Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional Fix: Fix a small Spec bug #52

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Optional Fix: Fix a small Spec bug #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38b32cb
chore: change to properties and code | tests are BROKEN
GalloDaSballo Oct 19, 2024
b89c8b3
Merge branch 'dev' into exp-unregisterable-fsm
GalloDaSballo Oct 21, 2024
9d0a09b
fix: tests that were valid
GalloDaSballo Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Governance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
64 changes: 24 additions & 40 deletions test/Governance.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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);

Expand All @@ -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");
Expand All @@ -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(
Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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!!
Expand All @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
4 changes: 1 addition & 3 deletions test/GovernanceAttacks.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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();


Expand Down
2 changes: 1 addition & 1 deletion test/UserProxy.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down
35 changes: 0 additions & 35 deletions test/recon/CryticToFoundry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 11 additions & 3 deletions test/recon/properties/GovernanceProperties.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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");
}

}
Expand Down
Loading