From 62235ca816ce422f41f691861a8e08347a286daa Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Thu, 28 Nov 2024 17:07:24 +0700 Subject: [PATCH 1/3] fix: incomplete events Add missing `boldAccrued` to `SnapshotVotes`. Add missing `vetos` to `SnapshotVotesForInitiative`. Add missing success flags when calling hooks. Add `indexed` to select events. Had to slightly refactor `_allocateLQTY()` to avoid running into "stack too deep". Fixes CS-V2Gov-040. --- src/Governance.sol | 309 ++++++++++++++++++--------------- src/interfaces/IGovernance.sol | 16 +- 2 files changed, 173 insertions(+), 152 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index 9ad395a9..5ab7dba2 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -129,7 +129,11 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG // post-deployment if we so choose, by backdating the first epoch at least EPOCH_DURATION in the past. registeredInitiatives[_initiatives[i]] = 1; - emit RegisterInitiative(_initiatives[i], msg.sender, 1); + bool success = safeCallWithMinGas( + _initiatives[i], MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (1)) + ); + + emit RegisterInitiative(_initiatives[i], msg.sender, 1, success); } _renounceOwnership(); @@ -347,7 +351,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG votesSnapshot = snapshot; uint256 boldBalance = bold.balanceOf(address(this)); boldAccrued = (boldBalance < MIN_ACCRUAL) ? 0 : boldBalance; - emit SnapshotVotes(snapshot.votes, snapshot.forEpoch); + emit SnapshotVotes(snapshot.votes, snapshot.forEpoch, boldAccrued); } } @@ -374,8 +378,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG } } - // Snapshots votes for an initiative for the previous epoch but only count the votes - // if the received votes meet the voting threshold + // Snapshots votes for an initiative for the previous epoch function _snapshotVotesForInitiative(address _initiative) internal returns (InitiativeVoteSnapshot memory initiativeSnapshot, InitiativeState memory initiativeState) @@ -385,7 +388,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG if (shouldUpdate) { votesForInitiativeSnapshot[_initiative] = initiativeSnapshot; - emit SnapshotVotesForInitiative(_initiative, initiativeSnapshot.votes, initiativeSnapshot.forEpoch); + emit SnapshotVotesForInitiative( + _initiative, initiativeSnapshot.votes, initiativeSnapshot.vetos, initiativeSnapshot.forEpoch + ); } } @@ -575,12 +580,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG /// @audit This ensures that the initiatives has UNREGISTRATION_AFTER_EPOCHS even after the first epoch initiativeStates[_initiative].lastEpochClaim = currentEpoch - 1; - emit RegisterInitiative(_initiative, msg.sender, currentEpoch); - // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas( + bool success = safeCallWithMinGas( _initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch)) ); + + emit RegisterInitiative(_initiative, msg.sender, currentEpoch, success); } struct ResetInitiativeData { @@ -707,6 +712,14 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG _allocateLQTY(_initiatives, _absoluteLQTYVotes, _absoluteLQTYVetos); } + // Avoid "stack too deep" by placing these variables in memory + struct AllocateLQTYVars { + VoteSnapshot votesSnapshot_; + GlobalState state; + uint16 currentEpoch; + UserState userState; + } + /// @dev For each given initiative applies relative changes to the allocation /// NOTE: Given the current usage the function either: Resets the value to 0, or sets the value to a new value /// Review the flows as the function could be used in many ways, but it ends up being used in just those 2 ways @@ -720,154 +733,162 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG "Governance: array-length-mismatch" ); - (VoteSnapshot memory votesSnapshot_, GlobalState memory state) = _snapshotVotes(); - uint16 currentEpoch = epoch(); - UserState memory userState = userStates[msg.sender]; + AllocateLQTYVars memory vars; + (vars.votesSnapshot_, vars.state) = _snapshotVotes(); + vars.currentEpoch = epoch(); + vars.userState = userStates[msg.sender]; for (uint256 i = 0; i < _initiatives.length; i++) { - address initiative = _initiatives[i]; - int88 deltaLQTYVotes = _deltaLQTYVotes[i]; - int88 deltaLQTYVetos = _deltaLQTYVetos[i]; - assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0); - - /// === Check FSM === /// - // Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states - // Force to remove votes if disabled - // Can remove votes and vetos in every stage - (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) = - _snapshotVotesForInitiative(initiative); - - (InitiativeStatus status,,) = - getInitiativeState(initiative, votesSnapshot_, votesForInitiativeSnapshot_, initiativeState); - - if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) { - /// @audit You cannot vote on `unregisterable` but a vote may have been there - require( - status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE - || status == InitiativeStatus.CLAIMED, - "Governance: active-vote-fsm" - ); - } + _allocateLQTYToInitiative(_initiatives[i], _deltaLQTYVotes[i], _deltaLQTYVetos[i], vars); + } - if (status == InitiativeStatus.DISABLED) { - require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal"); - } + require( + vars.userState.allocatedLQTY == 0 + || vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))), + "Governance: insufficient-or-allocated-lqty" + ); - /// === UPDATE ACCOUNTING === /// - // == INITIATIVE STATE == // + globalState = vars.state; + userStates[msg.sender] = vars.userState; + } - // deep copy of the initiative's state before the allocation - InitiativeState memory prevInitiativeState = InitiativeState( - initiativeState.voteLQTY, - initiativeState.vetoLQTY, - initiativeState.averageStakingTimestampVoteLQTY, - initiativeState.averageStakingTimestampVetoLQTY, - initiativeState.lastEpochClaim - ); + function _allocateLQTYToInitiative( + address initiative, + int88 deltaLQTYVotes, + int88 deltaLQTYVetos, + AllocateLQTYVars memory vars + ) internal { + assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0); - // update the average staking timestamp for the initiative based on the user's average staking timestamp - initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp( - initiativeState.averageStakingTimestampVoteLQTY, - userState.averageStakingTimestamp, - /// @audit This is wrong unless we enforce a reset on deposit and withdrawal - initiativeState.voteLQTY, - add(initiativeState.voteLQTY, deltaLQTYVotes) - ); - initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp( - initiativeState.averageStakingTimestampVetoLQTY, - userState.averageStakingTimestamp, - /// @audit This is wrong unless we enforce a reset on deposit and withdrawal - initiativeState.vetoLQTY, - add(initiativeState.vetoLQTY, deltaLQTYVetos) - ); + /// === Check FSM === /// + // Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states + // Force to remove votes if disabled + // Can remove votes and vetos in every stage + (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) = + _snapshotVotesForInitiative(initiative); - // 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 state - initiativeStates[initiative] = initiativeState; - - // == GLOBAL STATE == // - - // TODO: Veto reducing total votes logic change - // TODO: Accounting invariants - // TODO: Let's say I want to cap the votes vs weights - // Then by definition, I add the effective LQTY - // And the effective TS - // I remove the previous one - // and add the next one - // Veto > Vote - // Reduce down by Vote (cap min) - // If Vote > Veto - // Increase by Veto - Veto (reduced max) - - // update the average staking timestamp for all counted voting LQTY - /// Discount previous only if the initiative was not unregistered - - /// @audit We update the state only for non-disabled initiaitives - /// Disabled initiaitves have had their totals subtracted already - /// Math is also non associative so we cannot easily compare values - if (status != InitiativeStatus.DISABLED) { - /// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0` - /// Removing votes from state desynchs the state until all users remove their votes from the initiative - /// The invariant that holds is: the one that removes the initiatives that have been unregistered - state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( - state.countedVoteLQTYAverageTimestamp, - prevInitiativeState.averageStakingTimestampVoteLQTY, - /// @audit We don't have a test that fails when this line is changed - state.countedVoteLQTY, - state.countedVoteLQTY - prevInitiativeState.voteLQTY - ); - assert(state.countedVoteLQTY >= prevInitiativeState.voteLQTY); - /// @audit INVARIANT: Never overflows - state.countedVoteLQTY -= prevInitiativeState.voteLQTY; - - state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( - state.countedVoteLQTYAverageTimestamp, - initiativeState.averageStakingTimestampVoteLQTY, - state.countedVoteLQTY, - state.countedVoteLQTY + initiativeState.voteLQTY - ); - - state.countedVoteLQTY += initiativeState.voteLQTY; - } + (InitiativeStatus status,,) = + getInitiativeState(initiative, vars.votesSnapshot_, votesForInitiativeSnapshot_, initiativeState); + + if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) { + /// @audit You cannot vote on `unregisterable` but a vote may have been there + require( + status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE + || status == InitiativeStatus.CLAIMED, + "Governance: active-vote-fsm" + ); + } - // == USER ALLOCATION == // + if (status == InitiativeStatus.DISABLED) { + require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal"); + } - // allocate the voting and vetoing LQTY to the initiative - Allocation memory allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative]; - allocation.voteLQTY = add(allocation.voteLQTY, deltaLQTYVotes); - allocation.vetoLQTY = add(allocation.vetoLQTY, deltaLQTYVetos); - allocation.atEpoch = currentEpoch; - require(!(allocation.voteLQTY != 0 && allocation.vetoLQTY != 0), "Governance: vote-and-veto"); - lqtyAllocatedByUserToInitiative[msg.sender][initiative] = allocation; + /// === UPDATE ACCOUNTING === /// + // == INITIATIVE STATE == // - // == USER STATE == // + // deep copy of the initiative's state before the allocation + InitiativeState memory prevInitiativeState = InitiativeState( + initiativeState.voteLQTY, + initiativeState.vetoLQTY, + initiativeState.averageStakingTimestampVoteLQTY, + initiativeState.averageStakingTimestampVetoLQTY, + initiativeState.lastEpochClaim + ); - userState.allocatedLQTY = add(userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos); + // update the average staking timestamp for the initiative based on the user's average staking timestamp + initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp( + initiativeState.averageStakingTimestampVoteLQTY, + vars.userState.averageStakingTimestamp, + /// @audit This is wrong unless we enforce a reset on deposit and withdrawal + initiativeState.voteLQTY, + add(initiativeState.voteLQTY, deltaLQTYVotes) + ); + initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp( + initiativeState.averageStakingTimestampVetoLQTY, + vars.userState.averageStakingTimestamp, + /// @audit This is wrong unless we enforce a reset on deposit and withdrawal + initiativeState.vetoLQTY, + add(initiativeState.vetoLQTY, deltaLQTYVetos) + ); - emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch); + // 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 state + initiativeStates[initiative] = initiativeState; + + // == GLOBAL STATE == // + + // TODO: Veto reducing total votes logic change + // TODO: Accounting invariants + // TODO: Let's say I want to cap the votes vs weights + // Then by definition, I add the effective LQTY + // And the effective TS + // I remove the previous one + // and add the next one + // Veto > Vote + // Reduce down by Vote (cap min) + // If Vote > Veto + // Increase by Veto - Veto (reduced max) + + // update the average staking timestamp for all counted voting LQTY + /// Discount previous only if the initiative was not unregistered + + /// @audit We update the state only for non-disabled initiaitives + /// Disabled initiaitves have had their totals subtracted already + /// Math is also non associative so we cannot easily compare values + if (status != InitiativeStatus.DISABLED) { + /// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0` + /// Removing votes from state desynchs the state until all users remove their votes from the initiative + /// The invariant that holds is: the one that removes the initiatives that have been unregistered + vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( + vars.state.countedVoteLQTYAverageTimestamp, + prevInitiativeState.averageStakingTimestampVoteLQTY, + /// @audit We don't have a test that fails when this line is changed + vars.state.countedVoteLQTY, + vars.state.countedVoteLQTY - prevInitiativeState.voteLQTY + ); + assert(vars.state.countedVoteLQTY >= prevInitiativeState.voteLQTY); + /// @audit INVARIANT: Never overflows + vars.state.countedVoteLQTY -= prevInitiativeState.voteLQTY; - // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas( - initiative, - MIN_GAS_TO_HOOK, - 0, - abi.encodeCall( - IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, userState, allocation, initiativeState) - ) + vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( + vars.state.countedVoteLQTYAverageTimestamp, + initiativeState.averageStakingTimestampVoteLQTY, + vars.state.countedVoteLQTY, + vars.state.countedVoteLQTY + initiativeState.voteLQTY ); + + vars.state.countedVoteLQTY += initiativeState.voteLQTY; } - require( - userState.allocatedLQTY == 0 - || userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))), - "Governance: insufficient-or-allocated-lqty" + // == USER ALLOCATION == // + + // allocate the voting and vetoing LQTY to the initiative + Allocation memory allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative]; + allocation.voteLQTY = add(allocation.voteLQTY, deltaLQTYVotes); + allocation.vetoLQTY = add(allocation.vetoLQTY, deltaLQTYVetos); + allocation.atEpoch = vars.currentEpoch; + require(!(allocation.voteLQTY != 0 && allocation.vetoLQTY != 0), "Governance: vote-and-veto"); + lqtyAllocatedByUserToInitiative[msg.sender][initiative] = allocation; + + // == USER STATE == // + + vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos); + + // Replaces try / catch | Enforces sufficient gas is passed + bool success = safeCallWithMinGas( + initiative, + MIN_GAS_TO_HOOK, + 0, + abi.encodeCall( + IInitiative.onAfterAllocateLQTY, + (vars.currentEpoch, msg.sender, vars.userState, allocation, initiativeState) + ) ); - globalState = state; - userStates[msg.sender] = userState; + emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, vars.currentEpoch, success); } /// @inheritdoc IGovernance @@ -909,12 +930,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG /// weeks * 2^16 > u32 so the contract will stop working before this is an issue registeredInitiatives[_initiative] = UNREGISTERED_INITIATIVE; - emit UnregisterInitiative(_initiative, currentEpoch); - // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas( + bool success = safeCallWithMinGas( _initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch)) ); + + emit UnregisterInitiative(_initiative, currentEpoch, success); } /// @inheritdoc IGovernance @@ -950,16 +971,16 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG bold.safeTransfer(_initiative, claimableAmount); - emit ClaimForInitiative(_initiative, claimableAmount, votesSnapshot_.forEpoch); - // Replaces try / catch | Enforces sufficient gas is passed - safeCallWithMinGas( + bool success = safeCallWithMinGas( _initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claimableAmount)) ); + emit ClaimForInitiative(_initiative, claimableAmount, votesSnapshot_.forEpoch, success); + return claimableAmount; } diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 9d1fb76a..1d2cefe8 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -8,17 +8,17 @@ import {ILQTYStaking} from "./ILQTYStaking.sol"; import {PermitParams} from "../utils/Types.sol"; interface IGovernance { - event DepositLQTY(address user, uint256 depositedLQTY); - event WithdrawLQTY(address user, uint256 withdrawnLQTY, uint256 accruedLUSD, uint256 accruedETH); + event DepositLQTY(address indexed user, uint256 depositedLQTY); + event WithdrawLQTY(address indexed user, uint256 withdrawnLQTY, uint256 accruedLUSD, uint256 accruedETH); - event SnapshotVotes(uint240 votes, uint16 forEpoch); - event SnapshotVotesForInitiative(address initiative, uint240 votes, uint16 forEpoch); + event SnapshotVotes(uint256 votes, uint256 forEpoch, uint256 boldAccrued); + event SnapshotVotesForInitiative(address indexed initiative, uint256 votes, uint256 vetos, uint256 forEpoch); - event RegisterInitiative(address initiative, address registrant, uint16 atEpoch); - event UnregisterInitiative(address initiative, uint16 atEpoch); + event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, bool hookSuccess); + event UnregisterInitiative(address initiative, uint256 atEpoch, bool hookSuccess); - event AllocateLQTY(address user, address initiative, int256 deltaVoteLQTY, int256 deltaVetoLQTY, uint16 atEpoch); - event ClaimForInitiative(address initiative, uint256 bold, uint256 forEpoch); + event AllocateLQTY(address indexed user, address indexed initiative, int256 deltaVoteLQTY, int256 deltaVetoLQTY, uint256 atEpoch, bool hookSuccess); + event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, bool hookSuccess); struct Configuration { uint128 registrationFee; From f632a9cfa78abf5642d9ee6cea651b9b3eaef099 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Thu, 28 Nov 2024 17:15:34 +0700 Subject: [PATCH 2/3] chore: forge fmt --- src/interfaces/IGovernance.sol | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 1d2cefe8..9104b514 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -17,7 +17,14 @@ interface IGovernance { event RegisterInitiative(address initiative, address registrant, uint256 atEpoch, bool hookSuccess); event UnregisterInitiative(address initiative, uint256 atEpoch, bool hookSuccess); - event AllocateLQTY(address indexed user, address indexed initiative, int256 deltaVoteLQTY, int256 deltaVetoLQTY, uint256 atEpoch, bool hookSuccess); + event AllocateLQTY( + address indexed user, + address indexed initiative, + int256 deltaVoteLQTY, + int256 deltaVetoLQTY, + uint256 atEpoch, + bool hookSuccess + ); event ClaimForInitiative(address indexed initiative, uint256 bold, uint256 forEpoch, bool hookSuccess); struct Configuration { From 17e7be13d9ee95378e018d7c38d7ce7314d72b50 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Tue, 3 Dec 2024 10:50:33 +0700 Subject: [PATCH 3/3] refactor: try to fix stack-too-deep with less diff noise --- src/Governance.sol | 271 ++++++++++++++++++++++----------------------- 1 file changed, 134 insertions(+), 137 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index 5ab7dba2..7db83361 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -713,11 +713,14 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG } // Avoid "stack too deep" by placing these variables in memory - struct AllocateLQTYVars { + struct AllocateLQTYMemory { VoteSnapshot votesSnapshot_; GlobalState state; - uint16 currentEpoch; UserState userState; + InitiativeVoteSnapshot votesForInitiativeSnapshot_; + InitiativeState initiativeState; + InitiativeState prevInitiativeState; + Allocation allocation; } /// @dev For each given initiative applies relative changes to the allocation @@ -733,162 +736,156 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG "Governance: array-length-mismatch" ); - AllocateLQTYVars memory vars; + AllocateLQTYMemory memory vars; (vars.votesSnapshot_, vars.state) = _snapshotVotes(); - vars.currentEpoch = epoch(); + uint16 currentEpoch = epoch(); vars.userState = userStates[msg.sender]; for (uint256 i = 0; i < _initiatives.length; i++) { - _allocateLQTYToInitiative(_initiatives[i], _deltaLQTYVotes[i], _deltaLQTYVetos[i], vars); - } + address initiative = _initiatives[i]; + int88 deltaLQTYVotes = _deltaLQTYVotes[i]; + int88 deltaLQTYVetos = _deltaLQTYVetos[i]; + assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0); + + /// === Check FSM === /// + // Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states + // Force to remove votes if disabled + // Can remove votes and vetos in every stage + (vars.votesForInitiativeSnapshot_, vars.initiativeState) = _snapshotVotesForInitiative(initiative); + + (InitiativeStatus status,,) = getInitiativeState( + initiative, vars.votesSnapshot_, vars.votesForInitiativeSnapshot_, vars.initiativeState + ); - require( - vars.userState.allocatedLQTY == 0 - || vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))), - "Governance: insufficient-or-allocated-lqty" - ); + if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) { + /// @audit You cannot vote on `unregisterable` but a vote may have been there + require( + status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE + || status == InitiativeStatus.CLAIMED, + "Governance: active-vote-fsm" + ); + } - globalState = vars.state; - userStates[msg.sender] = vars.userState; - } + if (status == InitiativeStatus.DISABLED) { + require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal"); + } - function _allocateLQTYToInitiative( - address initiative, - int88 deltaLQTYVotes, - int88 deltaLQTYVetos, - AllocateLQTYVars memory vars - ) internal { - assert(deltaLQTYVotes != 0 || deltaLQTYVetos != 0); + /// === UPDATE ACCOUNTING === /// + // == INITIATIVE STATE == // - /// === Check FSM === /// - // Can vote positively in SKIP, CLAIMABLE, CLAIMED and UNREGISTERABLE states - // Force to remove votes if disabled - // Can remove votes and vetos in every stage - (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState) = - _snapshotVotesForInitiative(initiative); + // deep copy of the initiative's state before the allocation + vars.prevInitiativeState = InitiativeState( + vars.initiativeState.voteLQTY, + vars.initiativeState.vetoLQTY, + vars.initiativeState.averageStakingTimestampVoteLQTY, + vars.initiativeState.averageStakingTimestampVetoLQTY, + vars.initiativeState.lastEpochClaim + ); - (InitiativeStatus status,,) = - getInitiativeState(initiative, vars.votesSnapshot_, votesForInitiativeSnapshot_, initiativeState); - - if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) { - /// @audit You cannot vote on `unregisterable` but a vote may have been there - require( - status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE - || status == InitiativeStatus.CLAIMED, - "Governance: active-vote-fsm" + // update the average staking timestamp for the initiative based on the user's average staking timestamp + vars.initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp( + vars.initiativeState.averageStakingTimestampVoteLQTY, + vars.userState.averageStakingTimestamp, + /// @audit This is wrong unless we enforce a reset on deposit and withdrawal + vars.initiativeState.voteLQTY, + add(vars.initiativeState.voteLQTY, deltaLQTYVotes) + ); + vars.initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp( + vars.initiativeState.averageStakingTimestampVetoLQTY, + vars.userState.averageStakingTimestamp, + /// @audit This is wrong unless we enforce a reset on deposit and withdrawal + vars.initiativeState.vetoLQTY, + add(vars.initiativeState.vetoLQTY, deltaLQTYVetos) ); - } - if (status == InitiativeStatus.DISABLED) { - require(deltaLQTYVotes <= 0 && deltaLQTYVetos <= 0, "Must be a withdrawal"); - } + // allocate the voting and vetoing LQTY to the initiative + vars.initiativeState.voteLQTY = add(vars.initiativeState.voteLQTY, deltaLQTYVotes); + vars.initiativeState.vetoLQTY = add(vars.initiativeState.vetoLQTY, deltaLQTYVetos); + + // update the initiative's state + initiativeStates[initiative] = vars.initiativeState; + + // == GLOBAL STATE == // + + // TODO: Veto reducing total votes logic change + // TODO: Accounting invariants + // TODO: Let's say I want to cap the votes vs weights + // Then by definition, I add the effective LQTY + // And the effective TS + // I remove the previous one + // and add the next one + // Veto > Vote + // Reduce down by Vote (cap min) + // If Vote > Veto + // Increase by Veto - Veto (reduced max) + + // update the average staking timestamp for all counted voting LQTY + /// Discount previous only if the initiative was not unregistered + + /// @audit We update the state only for non-disabled initiaitives + /// Disabled initiaitves have had their totals subtracted already + /// Math is also non associative so we cannot easily compare values + if (status != InitiativeStatus.DISABLED) { + /// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0` + /// Removing votes from state desynchs the state until all users remove their votes from the initiative + /// The invariant that holds is: the one that removes the initiatives that have been unregistered + vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( + vars.state.countedVoteLQTYAverageTimestamp, + vars.prevInitiativeState.averageStakingTimestampVoteLQTY, + /// @audit We don't have a test that fails when this line is changed + vars.state.countedVoteLQTY, + vars.state.countedVoteLQTY - vars.prevInitiativeState.voteLQTY + ); + assert(vars.state.countedVoteLQTY >= vars.prevInitiativeState.voteLQTY); + /// @audit INVARIANT: Never overflows + vars.state.countedVoteLQTY -= vars.prevInitiativeState.voteLQTY; + + vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( + vars.state.countedVoteLQTYAverageTimestamp, + vars.initiativeState.averageStakingTimestampVoteLQTY, + vars.state.countedVoteLQTY, + vars.state.countedVoteLQTY + vars.initiativeState.voteLQTY + ); + + vars.state.countedVoteLQTY += vars.initiativeState.voteLQTY; + } - /// === UPDATE ACCOUNTING === /// - // == INITIATIVE STATE == // + // == USER ALLOCATION == // - // deep copy of the initiative's state before the allocation - InitiativeState memory prevInitiativeState = InitiativeState( - initiativeState.voteLQTY, - initiativeState.vetoLQTY, - initiativeState.averageStakingTimestampVoteLQTY, - initiativeState.averageStakingTimestampVetoLQTY, - initiativeState.lastEpochClaim - ); + // allocate the voting and vetoing LQTY to the initiative + vars.allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative]; + vars.allocation.voteLQTY = add(vars.allocation.voteLQTY, deltaLQTYVotes); + vars.allocation.vetoLQTY = add(vars.allocation.vetoLQTY, deltaLQTYVetos); + vars.allocation.atEpoch = currentEpoch; + require(!(vars.allocation.voteLQTY != 0 && vars.allocation.vetoLQTY != 0), "Governance: vote-and-veto"); + lqtyAllocatedByUserToInitiative[msg.sender][initiative] = vars.allocation; - // update the average staking timestamp for the initiative based on the user's average staking timestamp - initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp( - initiativeState.averageStakingTimestampVoteLQTY, - vars.userState.averageStakingTimestamp, - /// @audit This is wrong unless we enforce a reset on deposit and withdrawal - initiativeState.voteLQTY, - add(initiativeState.voteLQTY, deltaLQTYVotes) - ); - initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp( - initiativeState.averageStakingTimestampVetoLQTY, - vars.userState.averageStakingTimestamp, - /// @audit This is wrong unless we enforce a reset on deposit and withdrawal - initiativeState.vetoLQTY, - add(initiativeState.vetoLQTY, deltaLQTYVetos) - ); + // == USER STATE == // - // 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 state - initiativeStates[initiative] = initiativeState; - - // == GLOBAL STATE == // - - // TODO: Veto reducing total votes logic change - // TODO: Accounting invariants - // TODO: Let's say I want to cap the votes vs weights - // Then by definition, I add the effective LQTY - // And the effective TS - // I remove the previous one - // and add the next one - // Veto > Vote - // Reduce down by Vote (cap min) - // If Vote > Veto - // Increase by Veto - Veto (reduced max) - - // update the average staking timestamp for all counted voting LQTY - /// Discount previous only if the initiative was not unregistered - - /// @audit We update the state only for non-disabled initiaitives - /// Disabled initiaitves have had their totals subtracted already - /// Math is also non associative so we cannot easily compare values - if (status != InitiativeStatus.DISABLED) { - /// @audit Trophy: `test_property_sum_of_lqty_global_user_matches_0` - /// Removing votes from state desynchs the state until all users remove their votes from the initiative - /// The invariant that holds is: the one that removes the initiatives that have been unregistered - vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( - vars.state.countedVoteLQTYAverageTimestamp, - prevInitiativeState.averageStakingTimestampVoteLQTY, - /// @audit We don't have a test that fails when this line is changed - vars.state.countedVoteLQTY, - vars.state.countedVoteLQTY - prevInitiativeState.voteLQTY - ); - assert(vars.state.countedVoteLQTY >= prevInitiativeState.voteLQTY); - /// @audit INVARIANT: Never overflows - vars.state.countedVoteLQTY -= prevInitiativeState.voteLQTY; - - vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp( - vars.state.countedVoteLQTYAverageTimestamp, - initiativeState.averageStakingTimestampVoteLQTY, - vars.state.countedVoteLQTY, - vars.state.countedVoteLQTY + initiativeState.voteLQTY + vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos); + + // Replaces try / catch | Enforces sufficient gas is passed + bool success = safeCallWithMinGas( + initiative, + MIN_GAS_TO_HOOK, + 0, + abi.encodeCall( + IInitiative.onAfterAllocateLQTY, + (currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState) + ) ); - vars.state.countedVoteLQTY += initiativeState.voteLQTY; + emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch, success); } - // == USER ALLOCATION == // - - // allocate the voting and vetoing LQTY to the initiative - Allocation memory allocation = lqtyAllocatedByUserToInitiative[msg.sender][initiative]; - allocation.voteLQTY = add(allocation.voteLQTY, deltaLQTYVotes); - allocation.vetoLQTY = add(allocation.vetoLQTY, deltaLQTYVetos); - allocation.atEpoch = vars.currentEpoch; - require(!(allocation.voteLQTY != 0 && allocation.vetoLQTY != 0), "Governance: vote-and-veto"); - lqtyAllocatedByUserToInitiative[msg.sender][initiative] = allocation; - - // == USER STATE == // - - vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos); - - // Replaces try / catch | Enforces sufficient gas is passed - bool success = safeCallWithMinGas( - initiative, - MIN_GAS_TO_HOOK, - 0, - abi.encodeCall( - IInitiative.onAfterAllocateLQTY, - (vars.currentEpoch, msg.sender, vars.userState, allocation, initiativeState) - ) + require( + vars.userState.allocatedLQTY == 0 + || vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))), + "Governance: insufficient-or-allocated-lqty" ); - emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, vars.currentEpoch, success); + globalState = vars.state; + userStates[msg.sender] = vars.userState; } /// @inheritdoc IGovernance