Skip to content

Commit

Permalink
Merge pull request #112 from liquity/fix-offset-dust
Browse files Browse the repository at this point in the history 
fix: dust left in `unallocatedOffset` despite allocating all LQTY
  • Loading branch information
@danielattilasimon
danielattilasimon authored Jan 2, 2025
2 parents ffbf480 + 5d2d605 commit 9efb1d1
Show file tree
Hide file tree
Showing 2 changed files with 232 additions and 6 deletions.
36 changes: 31 additions & 5 deletions src/Governance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,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);
Expand Down Expand Up @@ -643,11 +644,23 @@ 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++) {
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
Expand Down Expand Up @@ -771,7 +784,11 @@ 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");
// 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 == //
Expand Down Expand Up @@ -900,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");
}
}
}
202 changes: 201 additions & 1 deletion test/Governance.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2373,6 +2376,203 @@ 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 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(1 ether);

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"
);
}

// 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
) 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(1 ether);

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);
Expand Down Expand Up @@ -2504,7 +2704,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);

Expand Down

0 comments on commit 9efb1d1

Please sign in to comment.