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

fix: dust left in unallocatedOffset despite allocating all LQTY #112

Merged
merged 8 commits into from
Jan 2, 2025
Merged

fix: dust left in unallocatedOffset despite allocating all LQTY #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ba0f44c
fix: dust left in `unallocatedOffset` despite allocating all LQTY
danielattilasimon Dec 25, 2024
9093f62
Merge remote-tracking branch 'origin/main' into fix-offset-dust
danielattilasimon Dec 30, 2024
e0b08a6
test: voting power manipulation via allocation
danielattilasimon Jan 1, 2025
8173873
test: voting power manipulation via withdrawal
danielattilasimon Jan 1, 2025
e50789c
fix: add strong assertions on non-negative voting power allocation
danielattilasimon Jan 1, 2025
3ca74e1
chore: add comment about offset allocation scheme
danielattilasimon Jan 1, 2025
73fbf86
chore: add explanation to rounding error test case
danielattilasimon Jan 2, 2025
5d2d605
chore: death to ugly numbers
danielattilasimon Jan 2, 2025
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
36 changes: 31 additions & 5 deletions src/Governance.sol
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!


// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it’s fine (specially because the difference should be almost negligible), but I wonder if we should put a comment mentioning that this is “path dependent” (related to the order of the array).
Let’s put an example with numbers. Let’s say unallocated LQTY is 10, unallocated offset is 5, and we allocate 3 and 7.
Before, we would have:

  • offset[0] = floor(3*5/10) = floor(1.5) = 1
  • offset[1] = floor(7*5/10) = floor(3.5) = 3

So there would be a remainder of 1 for the offset, when it should be zero.

Now, we’ll have:

  • offset[0] = floor(3*5/10) = floor(1.5) = 1
  • offset[1] = floor(7*4/7) = floor(4) = 4
    (as we first reduce unallocated amounts, from 10/5 to 7/4)

So now the final offset is zero as we want, but all the error is “accumulated” in the last initiative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is path-dependent. I agree it makes sense to document this.

I wouldn't say that all the error is accumulated in the last initiative, more like spread out over all initiatives. Consider an example with more than 2 initiatives. Let's say we have 4 LQTY and an offset of 3, which we want to split up equally between 4 initiatives.

Ideal:

offset[0] = 3*1/4 = 0.75
offset[1] = 3*1/4 = 0.75
offset[2] = 3*1/4 = 0.75
offset[3] = 3*1/4 = 0.75

Before:

offset[0] = floor(3*1/4) = floor(0.75) = 0 // error[0] = 0 - 0.75 = -0.75
offset[1] = floor(3*1/4) = floor(0.75) = 0 // error[1] = 0 - 0.75 = -0.75
offset[2] = floor(3*1/4) = floor(0.75) = 0 // error[2] = 0 - 0.75 = -0.75
offset[3] = floor(3*1/4) = floor(0.75) = 0 // error[3] = 0 - 0.75 = -0.75

Now:

offset[0] = floor(3*1/4) = floor(0.75) = 0 // error[0] = 0 - 0.75 = -0.75
offset[1] = floor(3*1/3) = floor(1.00) = 1 // error[1] = 1 - 0.75 =  0.25
offset[2] = floor(2*1/2) = floor(1.00) = 1 // error[2] = 1 - 0.75 =  0.25
offset[3] = floor(1*1/1) = floor(1.00) = 1 // error[3] = 1 - 0.75 =  0.25

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right! Which means the issue is even less bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now wondering if this is an improvement at all. Sure, the "average" offset error when allocating all of one's LQTY is zero, but it doesn't change the fact that there are (small) errors in the offsets allocated to the individual initiatives. Previously, the per-initiative error was in the (-1, 0] range, whereas now it's somewhere in (-1, 1).

Is this worth it? It's not like you can continue accumulating "dust" in unallocatedOffset, as we force you to fully deallocate before making any new allocations, which "resets" the accumulated error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point, but I still think that having unallocatedLQTY zero, but non-zero unallocatedOffset feels a little bit ugly. I wonder if having a positive error in the offset of the initiatives can introduce any new problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added assertions and tests to ensure this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment about path-dependency.

Copy link
Contributor

@RickGriff RickGriff Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone deposits LQTY then immediately allocates it to several initatives, their voting power assigned to each initatitive should be 0, but due to the rounding error, some will get positive and some will get negative voting power, right? (assuming no clamping)

It seems like either users or initatives need to bear the error.

Anyway still clamping lqtyToVotes, right? So in practice we don't get negative voting power. Given that, I'm happy with the solution for clearing the user's dust.

Copy link
Contributor Author

@danielattilasimon danielattilasimon Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When someone has zero unallocated voting power, it implies that their unallocated offset is a multiple of their unallocated LQTY, thus there can be no rounding error when allocating it to any number of initiatives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that makes sense!


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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer than the original double negation! ;-)

}
}
}
202 changes: 201 additions & 1 deletion test/Governance.t.sol
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test!

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
Loading