Voting power refactor with offsets (y-intercept approach) #97

RickGriff · 2024-12-04T16:54:51Z

All uints smaller than uint256 have been converted to uint256
All ints smaller than int256 converted to int256
Recon invariant properties converted to offset scheme
The timestamp approach has been converted to the offset (y-intercept) scheme. It fixes the "allocate->deallocate" path dependence. It follows this approach:
https://docs.google.com/document/d/1nPdD-w1n_0KIzAgi3Y5c8h2t2wkFiFD5YDHKsc6KxH8/edit?usp=sharing
Here's an example (see final allocate->deallocate actions by B for path independence):
https://docs.google.com/spreadsheets/d/1MSOsrIR-3qIwKPBlXkUTsQcfg7wwjfegCW8VD7QZWn8/edit?usp=sharing

TODO:

Fix unit tests so they run!
Fix merge conflicts
"Custom precision" has not been fixed - AFAICS, can just be removed now

bingen · 2024-12-05T19:37:49Z

src/interfaces/IGovernance.sol

-        int88[] memory _absoluteLQTYVotes,
-        int88[] memory absoluteLQTYVetos
+        int256[] memory _absoluteLQTYVotes,
+        int256[] memory absoluteLQTYVetos


Can’t these be unsigned ints?

Hmm yes. We'd need different conversion later on though, and I just kept everything the same on this front (including the requireNoNegatives check, which I suppose is silly for these quantities, but it was the original logic)

bingen · 2024-12-05T19:44:07Z

src/Governance.sol

        UserState memory userState = userStates[msg.sender];
-        require(userState.allocatedLQTY == 0, "Governance: must-be-zero-allocation");
+        require(userState.unallocatedLQTY == 0, "Governance: must-be-zero-allocation");


I don’t think I understand this change.

Right, this was a typo - fixed.

bingen · 2024-12-05T21:21:53Z

src/Governance.sol

@@ -739,93 +722,65 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
            // deep copy of the initiative's state before the allocation
            InitiativeState memory prevInitiativeState = InitiativeState(
                initiativeState.voteLQTY,
+                  initiativeState.voteOffset,


We can pass a forge fmt at the end 😉

bingen · 2024-12-05T21:22:55Z

src/Governance.sol

            // allocate the voting and vetoing LQTY to the initiative
            initiativeState.voteLQTY = add(initiativeState.voteLQTY, deltaLQTYVotes);
            initiativeState.vetoLQTY = add(initiativeState.vetoLQTY, deltaLQTYVetos);

+            // Update the initiative's vote and veto offsets
+            initiativeState.voteOffset = add(initiativeState.voteOffset, deltaOffsetVotes);
+            initiativeState.vetoOffset = add(initiativeState.vetoOffset, deltaOffsetVetos);


If we can make all uints, we’ll also get rid of this add here.

The deltas can be positive or negative though, since this internal function _allocateLQTY is used twice: first to deallocate everything to 0, secondly to reallocate to the chosen allocations. So the deltas are ints.

bingen · 2024-12-11T15:41:04Z

src/BribeInitiative.sol

-        uint256 scaledEpochEnd = (
-            governance.EPOCH_START() + _epoch * governance.EPOCH_DURATION()
-        ) * TIMESTAMP_PRECISION;
+        uint256 scaledEpochEnd = governance.EPOCH_START() + _epoch * governance.EPOCH_DURATION();


No big deal, but maybe we can get rid of the scaled part of the name?

bingen · 2024-12-11T15:46:01Z

src/Governance.sol

@@ -452,34 +448,19 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
        // == Rewards Conditions (votes can be zero, logic is the same) == //

        // By definition if _votesForInitiativeSnapshot.votes > 0 then _votesSnapshot.votes > 0
+        if (_votesForInitiativeSnapshot.votes > votingTheshold && _votesForInitiativeSnapshot.votes > _votesForInitiativeSnapshot.vetos) 
+        {
+            uint256 claim = _votesForInitiativeSnapshot.votes * boldAccrued / _votesSnapshot.votes;


Nice simplification!

Did we have a test case for what this custom precision was trying to solve? Are we sure we are not re-introducing any important rounding error?
Hopefully not thanks to the y-intercept refactor, because code looks much better now.

Good question. Will ask Alex which test exactly is relevant here.

bingen · 2024-12-11T15:53:42Z

src/BribeInitiative.sol

@@ -99,28 +97,18 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
            "BribeInitiative: invalid-prev-total-lqty-allocation-epoch"
        );

-        (uint88 totalLQTY, uint120 totalAverageTimestamp) = _decodeLQTYAllocation(totalLQTYAllocation.value);


Can we remove now utils/EncodingDecodingLib.sol?

bingen

I left some minor comments inline, but this looks very good to me.
Maybe the most important one is the one about int -> uint. I think it would make code nicer, specially for those “absolute” values, where it makes little sense to allow for negatives.

bingen · 2024-12-11T15:58:52Z

src/BribeInitiative.sol

-        ) * uint120(TIMESTAMP_PRECISION);
-
-        /// @audit User Invariant
-        assert(totalAverageTimestamp <= scaledEpochEnd);


Not sure if needed, or if we should move it to invariant tests, but I guess we could translate this into something like:

assert(totalLQTYAllocation.lqty * scaledEpochEnd - totalLQTYAllocation.offset >= 0);

(which is equivalent to assert(governance.lqtyToVotes(totalLQTYAllocation.lqty, scaledEpochEnd, totalLQTYAllocation.offset) >= 0);)

@danielattilasimon do you think we could add this to invariant tests? So we don’t pollute contracts with asserts?

bingen · 2024-12-11T16:02:15Z

src/BribeInitiative.sol


-            /// @audit Governance Invariant
-            assert(averageTimestamp <= scaledEpochEnd);


Same as previous assert comment.

bingen · 2024-12-11T17:18:53Z

src/Governance.sol

        // check that user has reset before changing lqty balance
        UserState storage userState = userStates[msg.sender];
        require(userState.allocatedLQTY == 0, "Governance: must-allocate-zero");

        UserProxy userProxy = UserProxy(payable(deriveUserProxyAddress(msg.sender)));
        require(address(userProxy).code.length != 0, "Governance: user-proxy-not-deployed");

-        uint88 lqtyStaked = uint88(stakingV1.stakes(address(userProxy)));
+         // Update the offset tracker
+        if (_lqtyAmount < userState.unallocatedLQTY) {


Are we checking that the input parameter is <= unallocatedLQTY? Otherwise it would revert below when trying to subtract.
Maybe we can update userState.unallocatedLQTY inside the if clause, zeroing it in the else branch, so we would be effectively clamping the input amount to the max.

Right, but if they try to withdraw more than their unallocatedLQTY, isn't revert the right behavior? But I'm fine to clamp to the max too

bingen · 2024-12-11T17:20:02Z

src/Governance.sol

@@ -542,17 +484,20 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
        // an initiative can be registered if the registrant has more voting power (LQTY * age)
        // than the registration threshold derived from the previous epoch's total global votes

-        uint256 upscaledSnapshotVotes = uint256(snapshot.votes);
+        uint256 upscaledSnapshotVotes = snapshot.votes;


Maybe we can get rid of the upscale part of the name.

It was happening when evaluating a user's voting power at a timestamp _before_ the first time they staked.

bingen reviewed Dec 5, 2024

View reviewed changes

bingen mentioned this pull request Dec 11, 2024

Overflow in claimBribes() #102

Merged

bingen reviewed Dec 11, 2024

View reviewed changes

bingen approved these changes Dec 11, 2024

View reviewed changes

RickGriff force-pushed the refactor_voting_power branch from 2d088c7 to d6cf089 Compare December 12, 2024 16:06

RickGriff and others added 23 commits December 14, 2024 22:16

Remove castings

6ce7c3e

Refactor timestamp approach to offset scheme

ec05967

Remove precision scaling and fix tests

98f8de4

Fix merge conflicts

3943468

forge fmt

d2f2052

rename variable

6c92420

fix: array access out-of-bounds

af790ba

fix: typo leading to underflows / bad accounting

c33b903

fix: overflow in token supply

1e0ad4c

fix: underflow in lqtyToVotes

e115a21

It was happening when evaluating a user's voting power at a timestamp _before_ the first time they staked.

test: fix multicall test

4f52510

fix: division by zero when user has no unallocated LQTY

06750f3

test: fix overflows in Governance fuzz tests

2b08ccc

test: fix test_allocateLQTY_single

345f4b0

chore: forge fmt

5302b2f

test: fix test_depositLQTYViaPermit

47c9806

test: fix test_depositLQTY_withdrawLQTY

dde67fd

test: fix test_voting_power_in_same_epoch_as_allocation

0b24a91

test: fix forked version of test_depositLQTY_withdrawLQTY

b13f1d7

test: fix test_offset_allocate_same_initiative_fuzz

27954c4

test: fix broken Governance property definition

e92e4e6

Fix BribeInitiative and GovernanceAttacks test suites

ea18e12

change uint and int sizes in test

3ac5be9

RickGriff force-pushed the refactor_voting_power branch from 14549ee to 3ac5be9 Compare December 14, 2024 15:55

chore: forge fmt

f37fda0

bingen merged commit d11e15a into main Dec 14, 2024
3 checks passed

danielattilasimon mentioned this pull request Dec 15, 2024

IR-15: Restore disabled test suites BribeInitiativeAllocate and VotingPower #103

Open

danielattilasimon deleted the refactor_voting_power branch January 19, 2025 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Voting power refactor with offsets (y-intercept approach) #97

Voting power refactor with offsets (y-intercept approach) #97

RickGriff commented Dec 4, 2024 •

edited

Loading

bingen Dec 5, 2024

RickGriff Dec 12, 2024

bingen Dec 5, 2024

RickGriff Dec 12, 2024

bingen Dec 5, 2024

RickGriff Dec 12, 2024

bingen Dec 5, 2024

RickGriff Dec 12, 2024

bingen Dec 11, 2024

bingen Dec 11, 2024

bingen Dec 11, 2024

RickGriff Dec 12, 2024

bingen Dec 11, 2024

RickGriff Dec 12, 2024

bingen left a comment

bingen Dec 11, 2024

bingen Dec 11, 2024

bingen Dec 11, 2024

bingen Dec 11, 2024

RickGriff Dec 12, 2024

bingen Dec 11, 2024

RickGriff Dec 12, 2024


		/// @audit Governance Invariant
		assert(averageTimestamp <= scaledEpochEnd);

Voting power refactor with offsets (y-intercept approach) #97

Voting power refactor with offsets (y-intercept approach) #97

Conversation

RickGriff commented Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RickGriff commented Dec 4, 2024 •

edited

Loading