From 17e7be13d9ee95378e018d7c38d7ce7314d72b50 Mon Sep 17 00:00:00 2001 From: Daniel Simon Date: Tue, 3 Dec 2024 10:50:33 +0700 Subject: [PATCH] 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 5ab7dba..7db8336 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