From ba0f44c9e91c2b19c60e2eda454ebbbfb37ec5fa Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Wed, 25 Dec 2024 12:31:28 +0700 Subject: [PATCH 1/7] fix: dust left in `unallocatedOffset` despite allocating all LQTY Closes #111. --- src/Governance.sol | 27 +++++++++++++--- test/Governance.t.sol | 73 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index b32946f..d4ef33f 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -606,6 +606,7 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own _requireNoNegatives(_absoluteLQTYVetos); // If the goal is to remove all votes from an initiative, including in _initiativesToReset is enough _requireNoNOP(_absoluteLQTYVotes, _absoluteLQTYVetos); + _requireNoSimultaneousVoteAndVeto(_absoluteLQTYVotes, _absoluteLQTYVetos); // You MUST always reset ResetInitiativeData[] memory cachedData = _resetInitiatives(_initiativesToReset); @@ -649,10 +650,18 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own // Calculate the offset portions that correspond to each LQTY vote and veto portion for (uint256 x; x < _initiatives.length; x++) { - absoluteOffsetVotes[x] = - _absoluteLQTYVotes[x] * int256(userState.unallocatedOffset) / int256(userState.unallocatedLQTY); - absoluteOffsetVetos[x] = - _absoluteLQTYVetos[x] * int256(userState.unallocatedOffset) / int256(userState.unallocatedLQTY); + // Either _absoluteLQTYVotes[x] or _absoluteLQTYVetos[x] is guaranteed to be zero + (int256[] calldata lqtyAmounts, int256[] memory offsets) = _absoluteLQTYVotes[x] > 0 + ? (_absoluteLQTYVotes, absoluteOffsetVotes) + : (_absoluteLQTYVetos, absoluteOffsetVetos); + + uint256 lqtyAmount = uint256(lqtyAmounts[x]); + uint256 offset = userState.unallocatedOffset * lqtyAmount / userState.unallocatedLQTY; + + userState.unallocatedLQTY -= lqtyAmount; + userState.unallocatedOffset -= offset; + + offsets[x] = int256(offset); } // Vote here, all values are now absolute changes @@ -780,7 +789,6 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own vars.allocation.atEpoch = vars.currentEpoch; - require(!(vars.allocation.voteLQTY != 0 && vars.allocation.vetoLQTY != 0), "Governance: vote-and-veto"); lqtyAllocatedByUserToInitiative[msg.sender][initiative] = vars.allocation; // == USER STATE == // @@ -909,4 +917,13 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own require(_absoluteLQTYVotes[i] > 0 || _absoluteLQTYVetos[i] > 0, "Governance: voting nothing"); } } + + function _requireNoSimultaneousVoteAndVeto(int256[] memory _absoluteLQTYVotes, int256[] memory _absoluteLQTYVetos) + internal + pure + { + for (uint256 i; i < _absoluteLQTYVotes.length; i++) { + require(_absoluteLQTYVotes[i] == 0 || _absoluteLQTYVetos[i] == 0, "Governance: vote-and-veto"); + } + } } diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 55749aa..67740f8 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -6,6 +6,7 @@ import {VmSafe} from "forge-std/Vm.sol"; import {console} from "forge-std/console.sol"; import {IERC20Errors} from "openzeppelin/contracts/interfaces/draft-IERC6093.sol"; +import {Strings} from "openzeppelin/contracts/utils/Strings.sol"; import {IGovernance} from "../src/interfaces/IGovernance.sol"; import {ILUSD} from "../src/interfaces/ILUSD.sol"; @@ -52,6 +53,8 @@ contract GovernanceTester is Governance { } abstract contract GovernanceTest is Test { + using Strings for uint256; + ILQTY internal lqty; ILUSD internal lusd; ILQTYStaking internal stakingV1; @@ -2287,6 +2290,74 @@ abstract contract GovernanceTest is Test { assertEq(votes, currentInitiativePower, "voting power of initiative should not be affected by vetos"); } + struct StakingOp { + uint256 lqtyAmount; + uint256 waitTime; + } + + function test_NoDustInUnallocatedOffsetAfterAllocatingAllLQTY(uint256[3] memory _votes, StakingOp[4] memory _stakes) + external + { + address[] memory initiatives = new address[](_votes.length + 1); + + // Ensure initiatives can be registered + vm.warp(block.timestamp + 2 * EPOCH_DURATION); + + // Register as many initiatives as needed + vm.startPrank(lusdHolder); + for (uint256 i = 0; i < initiatives.length; ++i) { + initiatives[i] = makeAddr(string.concat("initiative", i.toString())); + lusd.approve(address(governance), REGISTRATION_FEE); + governance.registerInitiative(initiatives[i]); + } + vm.stopPrank(); + + // Ensure the new initiatives are votable + vm.warp(block.timestamp + EPOCH_DURATION); + + vm.startPrank(user); + { + // Don't wait too long or initiatives might time out + uint256 maxWaitTime = EPOCH_DURATION * UNREGISTRATION_AFTER_EPOCHS / _stakes.length; + address userProxy = governance.deriveUserProxyAddress(user); + uint256 lqtyBalance = lqty.balanceOf(user); + uint256 unallocatedLQTY_ = 0; + + for (uint256 i = 0; i < _stakes.length; ++i) { + _stakes[i].lqtyAmount = _bound(_stakes[i].lqtyAmount, 1, lqtyBalance - (_stakes.length - 1 - i)); + lqtyBalance -= _stakes[i].lqtyAmount; + unallocatedLQTY_ += _stakes[i].lqtyAmount; + + lqty.approve(userProxy, _stakes[i].lqtyAmount); + governance.depositLQTY(_stakes[i].lqtyAmount); + + _stakes[i].waitTime = _bound(_stakes[i].waitTime, 1, maxWaitTime); + vm.warp(block.timestamp + _stakes[i].waitTime); + } + + address[] memory initiativesToReset; // left empty + int256[] memory votes = new int256[](initiatives.length); + int256[] memory vetos = new int256[](initiatives.length); // left zero + + for (uint256 i = 0; i < initiatives.length - 1; ++i) { + uint256 vote = _bound(_votes[i], 1, unallocatedLQTY_ - (initiatives.length - 1 - i)); + unallocatedLQTY_ -= vote; + votes[i] = int256(vote); + } + + // Cast all remaining LQTY on the last initiative + votes[initiatives.length - 1] = int256(unallocatedLQTY_); + + vm.assume(governance.secondsWithinEpoch() < EPOCH_VOTING_CUTOFF); + governance.allocateLQTY(initiativesToReset, initiatives, votes, vetos); + } + vm.stopPrank(); + + (uint256 unallocatedLQTY, uint256 unallocatedOffset,,) = governance.userStates(user); + assertEqDecimal(unallocatedLQTY, 0, 18, "user should have no unallocated LQTY"); + assertEqDecimal(unallocatedOffset, 0, 18, "user should have no unallocated offset"); + } + function _stakeLQTY(address staker, uint256 amount) internal { vm.startPrank(staker); address userProxy = governance.deriveUserProxyAddress(staker); @@ -2370,7 +2441,7 @@ contract MockedGovernanceTest is GovernanceTest, MockStakingV1Deployer { function setUp() public override { (MockStakingV1 mockStakingV1, MockERC20Tester mockLQTY, MockERC20Tester mockLUSD) = deployMockStakingV1(); - mockLQTY.mint(user, 1_000e18); + mockLQTY.mint(user, 10_000e18); mockLQTY.mint(user2, 1_000e18); mockLUSD.mint(lusdHolder, 20_000e18); From e0b08a6c9b26ba9215af36c659b96ff9c85cc4d8 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Wed, 1 Jan 2025 08:10:38 +0700 Subject: [PATCH 2/7] test: voting power manipulation via allocation --- test/Governance.t.sol | 59 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 28da77f..e414430 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -2444,6 +2444,65 @@ abstract contract GovernanceTest is Test { assertEqDecimal(unallocatedOffset, 0, 18, "user should have no unallocated offset"); } + function test_WhenAllocatingTinyAmounts_VotingPowerDoesNotTurnNegativeDueToRoundingError( + uint256 initialVotingPower, + uint256 numInitiatives + ) external { + initialVotingPower = bound(initialVotingPower, 1, 20); + numInitiatives = bound(numInitiatives, 1, 20); + + address[] memory initiatives = new address[](numInitiatives); + + // Ensure initiatives can be registered + vm.warp(block.timestamp + 2 * EPOCH_DURATION); + + // Register as many initiatives as needed + vm.startPrank(lusdHolder); + for (uint256 i = 0; i < initiatives.length; ++i) { + initiatives[i] = makeAddr(string.concat("initiative", i.toString())); + lusd.approve(address(governance), REGISTRATION_FEE); + governance.registerInitiative(initiatives[i]); + } + vm.stopPrank(); + + // Ensure the new initiatives are votable + vm.warp(block.timestamp + EPOCH_DURATION); + + vm.startPrank(user); + { + address userProxy = governance.deriveUserProxyAddress(user); + lqty.approve(userProxy, type(uint256).max); + governance.depositLQTY(1); + + // By waiting `initialVotingPower` seconds while having 1 wei LQTY staked, + // we accrue exactly `initialVotingPower` + vm.warp(block.timestamp + initialVotingPower); + governance.depositLQTY(583399417581888701); + + address[] memory initiativesToReset; // left empty + int256[] memory votes = new int256[](initiatives.length); + int256[] memory vetos = new int256[](initiatives.length); // left zero + + for (uint256 i = 0; i < initiatives.length; ++i) { + votes[i] = 1; + } + + governance.allocateLQTY(initiativesToReset, initiatives, votes, vetos); + } + vm.stopPrank(); + + (uint256 unallocatedLQTY, uint256 unallocatedOffset,,) = governance.userStates(user); + int256 votingPower = int256(unallocatedLQTY * block.timestamp) - int256(unallocatedOffset); + + // Even though we are allocating tiny amounts, each allocation + // reduces voting power by 1 (due to rounding), but not below zero + assertEq( + votingPower, + int256(initialVotingPower > numInitiatives ? initialVotingPower - numInitiatives : 0), + "voting power should stay non-negative" + ); + } + function test_Vote_Stake_Unvote() external { address[] memory noInitiatives; address[] memory initiatives = new address[](1); From 8173873e64ecd28ef1249eeba75e9bdcb616f983 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Wed, 1 Jan 2025 08:24:54 +0700 Subject: [PATCH 3/7] test: voting power manipulation via withdrawal --- test/Governance.t.sol | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index e414430..c38abb4 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -2477,6 +2477,7 @@ abstract contract GovernanceTest is Test { // By waiting `initialVotingPower` seconds while having 1 wei LQTY staked, // we accrue exactly `initialVotingPower` vm.warp(block.timestamp + initialVotingPower); + governance.depositLQTY(583399417581888701); address[] memory initiativesToReset; // left empty @@ -2503,6 +2504,43 @@ abstract contract GovernanceTest is Test { ); } + function test_WhenWithdrawingTinyAmounts_VotingPowerDoesNotTurnNegativeDueToRoundingError( + uint256 initialVotingPower, + uint256 numWithdrawals + ) external { + initialVotingPower = bound(initialVotingPower, 1, 20); + numWithdrawals = bound(numWithdrawals, 1, 20); + + vm.startPrank(user); + { + address userProxy = governance.deriveUserProxyAddress(user); + lqty.approve(userProxy, type(uint256).max); + governance.depositLQTY(1); + + // By waiting `initialVotingPower` seconds while having 1 wei LQTY staked, + // we accrue exactly `initialVotingPower` + vm.warp(block.timestamp + initialVotingPower); + + governance.depositLQTY(583399417581888701); + + for (uint256 i = 0; i < numWithdrawals; ++i) { + governance.withdrawLQTY(1); + } + } + vm.stopPrank(); + + (uint256 unallocatedLQTY, uint256 unallocatedOffset,,) = governance.userStates(user); + int256 votingPower = int256(unallocatedLQTY * block.timestamp) - int256(unallocatedOffset); + + // Even though we are withdrawing tiny amounts, each withdrawal + // reduces voting power by 1 (due to rounding), but not below zero + assertEq( + votingPower, + int256(initialVotingPower > numWithdrawals ? initialVotingPower - numWithdrawals : 0), + "voting power should stay non-negative" + ); + } + function test_Vote_Stake_Unvote() external { address[] memory noInitiatives; address[] memory initiatives = new address[](1); From e50789c23f17b9b7f3be4b6df3c09fbee6e678ce Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Wed, 1 Jan 2025 09:01:16 +0700 Subject: [PATCH 4/7] fix: add strong assertions on non-negative voting power allocation --- src/Governance.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Governance.sol b/src/Governance.sol index ad1b39b..1e44227 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -780,6 +780,11 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own vars.allocation.atEpoch = vars.currentEpoch; + // Voting power allocated to initiatives should never be negative, else it might break reward allocation + // schemes such as `BribeInitiative` which distribute rewards in proportion to voting power allocated. + assert(vars.allocation.voteLQTY * block.timestamp >= vars.allocation.voteOffset); + assert(vars.allocation.vetoLQTY * block.timestamp >= vars.allocation.vetoOffset); + lqtyAllocatedByUserToInitiative[msg.sender][initiative] = vars.allocation; // == USER STATE == // From 3ca74e1589c4c45e8747c605fd850764768166c6 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Wed, 1 Jan 2025 09:20:07 +0700 Subject: [PATCH 5/7] chore: add comment about offset allocation scheme --- src/Governance.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Governance.sol b/src/Governance.sol index 1e44227..b2dbf1d 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -644,6 +644,10 @@ contract Governance is MultiDelegateCall, UserProxyFactory, ReentrancyGuard, Own int256[] memory absoluteOffsetVetos = new int256[](_initiatives.length); // Calculate the offset portions that correspond to each LQTY vote and veto portion + // By recalculating `unallocatedLQTY` & `unallocatedOffset` after each step, we ensure that rounding error + // doesn't accumulate in `unallocatedOffset`. + // However, it should be noted that this makes the exact offset allocations dependent on the ordering of the + // `_initiatives` array. for (uint256 x; x < _initiatives.length; x++) { // Either _absoluteLQTYVotes[x] or _absoluteLQTYVetos[x] is guaranteed to be zero (int256[] calldata lqtyAmounts, int256[] memory offsets) = _absoluteLQTYVotes[x] > 0 From 73fbf86ea5ceef4626af295db00619d68af1e17d Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Thu, 2 Jan 2025 12:41:40 +0700 Subject: [PATCH 6/7] chore: add explanation to rounding error test case --- test/Governance.t.sol | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index c38abb4..afe068b 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -2504,6 +2504,38 @@ abstract contract GovernanceTest is Test { ); } + // We find that a user's unallocated voting power can't be turned negative through manipulation, which is + // demonstrated in the next test. + // + // Whenever a user withdraws LQTY, they can lose more voting power than they should, due to rounding error in the + // calculation of their remaining offset: + // + // unallocatedOffset -= FLOOR(lqtyDecrease * unallocatedOffset / unallocatedLQTY) + // unallocatedLQTY -= lqtyDecrease + // + // For reference, unallocated voting power at time `t` is calculated as: + // + // unallocatedLQTY * t - unallocatedOffset + // + // The decrement of `unallocatedOffset` is rounded down, consequently `unallocatedOffset` is rounded up, in turn the + // voting power is rounded down. So when time a user has some relatively small positive unallocated voting power and + // a significant amount of unallocated LQTY, and withdraws a tiny amount of LQTY (corresponding to less than a unit + // of voting power), they lose a full unit of voting power. + // + // One might think that this can be done repeatedly in an attempt to manipulate unallocated voting power into + // negative range, thus being able to allocate negative voting power to an initiative (if done very close to the + // end of the present epoch), which would be bad as it would result in insolvency in initiatives that distribute + // rewards in proportion to voting power allocated by voters (such as `BribeInitiative`). + // + // However, we find that this manipulation stops being effective once unallocated voting power reaches zero. Having + // zero unallocated voting power means: + // + // unallocatedLQTY * t - unallocatedOffset = 0 + // unallocatedLQTY * t = unallocatedOffset + // + // Thus when unallocated voting power is zero, `unallocatedOffset` is a multiple of `unallocatedLQTY`, so there can + // be no more rounding error when re-calculating `unallocatedOffset` on withdrawals. + function test_WhenWithdrawingTinyAmounts_VotingPowerDoesNotTurnNegativeDueToRoundingError( uint256 initialVotingPower, uint256 numWithdrawals From 5d2d6055fa093a07304140b55917ef3bd3f96b67 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Thu, 2 Jan 2025 12:45:28 +0700 Subject: [PATCH 7/7] chore: death to ugly numbers --- test/Governance.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index afe068b..234e359 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -2478,7 +2478,7 @@ abstract contract GovernanceTest is Test { // we accrue exactly `initialVotingPower` vm.warp(block.timestamp + initialVotingPower); - governance.depositLQTY(583399417581888701); + governance.depositLQTY(1 ether); address[] memory initiativesToReset; // left empty int256[] memory votes = new int256[](initiatives.length); @@ -2553,7 +2553,7 @@ abstract contract GovernanceTest is Test { // we accrue exactly `initialVotingPower` vm.warp(block.timestamp + initialVotingPower); - governance.depositLQTY(583399417581888701); + governance.depositLQTY(1 ether); for (uint256 i = 0; i < numWithdrawals; ++i) { governance.withdrawLQTY(1);