Skip to content

Commit

Permalink
Merge pull request #95 from liquity/no-immediate-registration
Browse files Browse the repository at this point in the history
feat: disallow registration before epoch #3
  • Loading branch information
danielattilasimon authored Dec 6, 2024
2 parents ce7e968 + f78e803 commit cafde08
Show file tree
Hide file tree
Showing 6 changed files with 204 additions and 37 deletions.
5 changes: 3 additions & 2 deletions src/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

/// @inheritdoc IGovernance
function registerInitiative(address _initiative) external nonReentrant {
uint16 currentEpoch = epoch();
require(currentEpoch > 2, "Governance: registration-not-yet-enabled");

bold.safeTransferFrom(msg.sender, address(this), REGISTRATION_FEE);

require(_initiative != address(0), "Governance: zero-address");
Expand All @@ -520,8 +523,6 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
"Governance: insufficient-lqty"
);

uint16 currentEpoch = epoch();

registeredInitiatives[_initiative] = currentEpoch;

/// This ensures that the initiatives has UNREGISTRATION_AFTER_EPOCHS even after the first epoch
Expand Down
165 changes: 165 additions & 0 deletions test/Deployment.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {IGovernance} from "../src/interfaces/IGovernance.sol";
import {Governance} from "../src/Governance.sol";
import {MockERC20Tester} from "./mocks/MockERC20Tester.sol";
import {MockStakingV1} from "./mocks/MockStakingV1.sol";
import {MockStakingV1Deployer} from "./mocks/MockStakingV1Deployer.sol";

// These tests demonstrate that by deploying `Governance` with `epochStart` set one `EPOCH_DURATION` in the past:
// - initial initiatives can immediately be voted on,
// - registration of new initiatives is disabled for one epoch.
//
// The reason we want to initially disable registration is that there's not vote snapshot to base the registration
// threshold upon, thus registration would otherwise be possible without having any LQTY staked.
contract DeploymentTest is MockStakingV1Deployer {
uint32 constant START_TIME = 1732873631;
uint32 constant EPOCH_DURATION = 7 days;
uint128 constant REGISTRATION_FEE = 1 ether;

address constant deployer = address(uint160(uint256(keccak256("deployer"))));
address constant voter = address(uint160(uint256(keccak256("voter"))));
address constant registrant = address(uint160(uint256(keccak256("registrant"))));
address constant initialInitiative = address(uint160(uint256(keccak256("initialInitiative"))));
address constant newInitiative = address(uint160(uint256(keccak256("newInitiative"))));

IGovernance.Configuration config = IGovernance.Configuration({
registrationFee: REGISTRATION_FEE,
registrationThresholdFactor: 0.01 ether,
unregistrationThresholdFactor: 4 ether,
unregistrationAfterEpochs: 4,
votingThresholdFactor: 0.04 ether,
minClaim: 0,
minAccrual: 0,
epochStart: START_TIME - EPOCH_DURATION,
epochDuration: EPOCH_DURATION,
epochVotingCutoff: EPOCH_DURATION - 1 days
});

MockStakingV1 stakingV1;
MockERC20Tester lqty;
MockERC20Tester lusd;
MockERC20Tester bold;
Governance governance;

address[] initiativesToReset;
address[] initiatives;
int88[] votes;
int88[] vetos;

function setUp() external {
vm.warp(START_TIME);

vm.label(deployer, "deployer");
vm.label(voter, "voter");
vm.label(registrant, "registrant");
vm.label(initialInitiative, "initialInitiative");
vm.label(newInitiative, "newInitiative");

(stakingV1, lqty, lusd) = deployMockStakingV1();
bold = new MockERC20Tester("BOLD Stablecoin", "BOLD");

initiatives.push(initialInitiative);

vm.prank(deployer);
governance = new Governance({
_lqty: address(lqty),
_lusd: address(lusd),
_stakingV1: address(stakingV1),
_bold: address(bold),
_config: config,
_owner: deployer,
_initiatives: initiatives
});

vm.label(governance.deriveUserProxyAddress(voter), "voterProxy");
}

function test_AtStart_WeAreInEpoch2() external view {
assertEq(governance.epoch(), 2, "We should start in epoch #2");
}

function test_OneEpochLater_WeAreInEpoch3() external {
vm.warp(block.timestamp + EPOCH_DURATION);
assertEq(governance.epoch(), 3, "We should be in epoch #3");
}

function test_AtStart_CanVoteOnInitialInitiative() external {
_voteOnInitiative();

uint256 boldAccrued = 1 ether;
bold.mint(address(governance), boldAccrued);
vm.warp(block.timestamp + EPOCH_DURATION);

governance.claimForInitiative(initialInitiative);
assertEqDecimal(bold.balanceOf(initialInitiative), boldAccrued, 18, "Initiative should have received BOLD");
}

function test_AtStart_CannotRegisterNewInitiative() external {
_registerNewInitiative({expectRevertReason: "Governance: registration-not-yet-enabled"});
}

function test_OneEpochLater_WhenNoOneVotedDuringEpoch2_CanRegisterNewInitiativeWithNoLQTY() external {
vm.warp(block.timestamp + EPOCH_DURATION);
_registerNewInitiative();
}

function test_OneEpochLater_WhenSomeoneVotedDuringEpoch2_CannotRegisterNewInitiativeWithNoLQTY() external {
_voteOnInitiative();
vm.warp(block.timestamp + EPOCH_DURATION);
_registerNewInitiative({expectRevertReason: "Governance: insufficient-lqty"});
_depositLQTY(); // Only LQTY deposited during previous epoch counts
_registerNewInitiative({expectRevertReason: "Governance: insufficient-lqty"});
}

function test_OneEpochLater_WhenSomeoneVotedDuringEpoch2_CanRegisterNewInitiativeWithSufficientLQTY() external {
_voteOnInitiative();
_depositLQTY();
vm.warp(block.timestamp + EPOCH_DURATION);
_registerNewInitiative();
}

/////////////
// Helpers //
/////////////

function _voteOnInitiative() internal {
uint88 lqtyAmount = 1 ether;
lqty.mint(voter, lqtyAmount);

votes.push(int88(lqtyAmount));
vetos.push(0);

vm.startPrank(voter);
lqty.approve(governance.deriveUserProxyAddress(voter), lqtyAmount);
governance.depositLQTY(lqtyAmount);
governance.allocateLQTY(initiativesToReset, initiatives, votes, vetos);
vm.stopPrank();

delete votes;
delete vetos;
}

function _registerNewInitiative() internal {
_registerNewInitiative("");
}

function _registerNewInitiative(bytes memory expectRevertReason) internal {
bold.mint(registrant, REGISTRATION_FEE);
vm.startPrank(registrant);
bold.approve(address(governance), REGISTRATION_FEE);
if (expectRevertReason.length > 0) vm.expectRevert(expectRevertReason);
governance.registerInitiative(newInitiative);
vm.stopPrank();
}

function _depositLQTY() internal {
uint88 lqtyAmount = 1 ether;
lqty.mint(registrant, lqtyAmount);
vm.startPrank(registrant);
lqty.approve(governance.deriveUserProxyAddress(registrant), lqtyAmount);
governance.depositLQTY(lqtyAmount);
vm.stopPrank();
}
}
16 changes: 13 additions & 3 deletions test/E2E.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ contract ForkedE2ETests is Test {
_allocate(baseInitiative1, 1e18, 0);
_reset(baseInitiative1);

// Registration not allowed initially, so skip one epoch
vm.warp(block.timestamp + EPOCH_DURATION);

deal(address(lusd), address(user), REGISTRATION_FEE);
lusd.approve(address(governance), REGISTRATION_FEE);
governance.registerInitiative(address(0x123123));
Expand Down Expand Up @@ -148,12 +151,15 @@ contract ForkedE2ETests is Test {
_deposit(1000e18);

console.log("epoch", governance.epoch());
_allocate(baseInitiative1, 1e18, 0); // Doesn't work due to cool down I think
_allocate(baseInitiative1, 1e18, 0);

// And for sanity, you cannot vote on new ones, they need to be added first
deal(address(lusd), address(user), REGISTRATION_FEE);
lusd.approve(address(governance), REGISTRATION_FEE);

// Registration not allowed initially, so skip one epoch
vm.warp(block.timestamp + EPOCH_DURATION);

address newInitiative = address(0x123123);
governance.registerInitiative(newInitiative);
assertEq(uint256(IGovernance.InitiativeStatus.WARM_UP), _getInitiativeStatus(newInitiative), "Cooldown");
Expand Down Expand Up @@ -192,7 +198,9 @@ contract ForkedE2ETests is Test {

// forge test --match-test test_unregisterWorksCorrectlyEvenAfterXEpochs -vv
function test_unregisterWorksCorrectlyEvenAfterXEpochs(uint8 epochsInFuture) public {
vm.warp(block.timestamp + epochsInFuture * EPOCH_DURATION);
// Registration starts working after one epoch, so fast-forward at least one EPOCH_DURATION
vm.warp(block.timestamp + (uint32(1) + epochsInFuture) * EPOCH_DURATION);

vm.startPrank(user);
// Check that we can vote on the first epoch, right after deployment
_deposit(1000e18);
Expand Down Expand Up @@ -251,7 +259,9 @@ contract ForkedE2ETests is Test {
}

function test_unregisterWorksCorrectlyEvenAfterXEpochs_andCanBeSavedAtLast(uint8 epochsInFuture) public {
vm.warp(block.timestamp + epochsInFuture * EPOCH_DURATION);
// Registration starts working after one epoch, so fast-forward at least one EPOCH_DURATION
vm.warp(block.timestamp + (uint32(1) + epochsInFuture) * EPOCH_DURATION);

vm.startPrank(user);
// Check that we can vote on the first epoch, right after deployment
_deposit(1000e18);
Expand Down
46 changes: 19 additions & 27 deletions test/Governance.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ abstract contract GovernanceTest is Test {
assertEq(governance.getLatestVotingThreshold(), 0);

// check that votingThreshold is is high enough such that MIN_CLAIM is met
IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1);
IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch());
governance.tester_setVotesSnapshot(snapshot);

uint256 boldAccrued = 1000e18;
Expand Down Expand Up @@ -382,7 +382,7 @@ abstract contract GovernanceTest is Test {
initialInitiatives
);

snapshot = IGovernance.VoteSnapshot(10000e18, 1);
snapshot = IGovernance.VoteSnapshot(10000e18, governance.epoch());
governance.tester_setVotesSnapshot(snapshot);

boldAccrued = 1000e18;
Expand Down Expand Up @@ -434,7 +434,14 @@ abstract contract GovernanceTest is Test {

address userProxy = governance.deployUserProxy();

IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1);
vm.expectRevert("Governance: registration-not-yet-enabled");
governance.registerInitiative(baseInitiative3);

// Registration not allowed before epoch #3
vm.warp(block.timestamp + 2 * EPOCH_DURATION);
assertEq(governance.epoch(), 3, "We should be in epoch #3");

IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch());
governance.tester_setVotesSnapshot(snapshot);

// should revert if the `REGISTRATION_FEE` > `lusd.balanceOf(msg.sender)`
Expand All @@ -459,7 +466,7 @@ abstract contract GovernanceTest is Test {

lqty.approve(address(userProxy), 1e18);
governance.depositLQTY(1e18);
vm.warp(block.timestamp + governance.EPOCH_DURATION());
vm.warp(block.timestamp + EPOCH_DURATION);

// should revert if `_initiative` is zero
vm.expectRevert("Governance: zero-address");
Expand All @@ -476,52 +483,37 @@ abstract contract GovernanceTest is Test {
vm.stopPrank();
}

// TODO: Broken: Fix it by simplifying most likely
// forge test --match-test test_unregisterInitiative -vv
function test_unregisterInitiative() public {
vm.startPrank(user);

address userProxy = governance.deployUserProxy();

IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1);
governance.tester_setVotesSnapshot(snapshot);

vm.stopPrank();

vm.startPrank(lusdHolder);
lusd.transfer(user, 1e18);
vm.stopPrank();

vm.startPrank(user);

lusd.approve(address(governance), 1e18);
lqty.approve(address(userProxy), 1e18);
governance.depositLQTY(1e18);
vm.warp(block.timestamp + governance.EPOCH_DURATION());

// should revert if the initiative isn't registered
vm.expectRevert("Governance: cannot-unregister-initiative");
governance.unregisterInitiative(baseInitiative3);

// Registration not allowed before epoch #3
vm.warp(block.timestamp + 2 * EPOCH_DURATION);
assertEq(governance.epoch(), 3, "We should be in epoch #3");

lusd.approve(address(governance), 1e18);
governance.registerInitiative(baseInitiative3);
uint16 atEpoch = governance.registeredInitiatives(baseInitiative3);
assertEq(atEpoch, governance.epoch());

// should revert if the initiative is still in the registration warm up period
vm.expectRevert("Governance: cannot-unregister-initiative");
/// @audit should fail due to not waiting enough time
governance.unregisterInitiative(baseInitiative3);

vm.warp(block.timestamp + governance.EPOCH_DURATION());
vm.warp(block.timestamp + EPOCH_DURATION);

// should revert if the initiative is still active or the vetos don't meet the threshold
vm.expectRevert("Governance: cannot-unregister-initiative");
governance.unregisterInitiative(baseInitiative3);

snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch() - 1);
governance.tester_setVotesSnapshot(snapshot);

vm.warp(block.timestamp + governance.EPOCH_DURATION() * UNREGISTRATION_AFTER_EPOCHS);
vm.warp(block.timestamp + EPOCH_DURATION * UNREGISTRATION_AFTER_EPOCHS);

governance.unregisterInitiative(baseInitiative3);

Expand Down Expand Up @@ -1467,7 +1459,7 @@ abstract contract GovernanceTest is Test {
address userProxy = governance.deployUserProxy();
IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1);
IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch());
governance.tester_setVotesSnapshot(snapshot);
vm.startPrank(lusdHolder);
Expand Down
5 changes: 2 additions & 3 deletions test/GovernanceAttacks.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ abstract contract GovernanceAttacksTest is Test {
votingThresholdFactor: VOTING_THRESHOLD_FACTOR,
minClaim: MIN_CLAIM,
minAccrual: MIN_ACCRUAL,
epochStart: uint32(block.timestamp),
// backdate by 2 epochs to ensure new initiatives can be registered from the start
epochStart: uint32(block.timestamp - 2 * EPOCH_DURATION),
epochDuration: EPOCH_DURATION,
epochVotingCutoff: EPOCH_VOTING_CUTOFF
});
Expand All @@ -70,8 +71,6 @@ abstract contract GovernanceAttacksTest is Test {

// All calls should never revert due to malicious initiative
function test_all_revert_attacks_hardcoded() public {
vm.warp(block.timestamp + governance.EPOCH_DURATION());

vm.startPrank(user);

// should not revert if the user doesn't have a UserProxy deployed yet
Expand Down
4 changes: 2 additions & 2 deletions test/recon/Setup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ abstract contract Setup is BaseSetup, MockStakingV1Deployer {
votingThresholdFactor: VOTING_THRESHOLD_FACTOR,
minClaim: MIN_CLAIM,
minAccrual: MIN_ACCRUAL,
epochStart: uint32(block.timestamp - EPOCH_DURATION),
/// @audit will this work?
// backdate by 2 epochs to ensure new initiatives can be registered from the start
epochStart: uint32(block.timestamp - 2 * EPOCH_DURATION),
epochDuration: EPOCH_DURATION,
epochVotingCutoff: EPOCH_VOTING_CUTOFF
}),
Expand Down

0 comments on commit cafde08

Please sign in to comment.