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: incomplete events [alternative version] #94

Merged
merged 3 commits into from
Dec 4, 2024
Merged

fix: incomplete events [alternative version] #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62235ca
fix: incomplete events
danielattilasimon Nov 28, 2024
f632a9c
chore: forge fmt
danielattilasimon Nov 28, 2024
17e7be1
refactor: try to fix stack-too-deep with less diff noise
danielattilasimon Dec 3, 2024
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
152 changes: 85 additions & 67 deletions src/Governance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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)
Expand All @@ -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
);
}
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -707,6 +712,17 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
_allocateLQTY(_initiatives, _absoluteLQTYVotes, _absoluteLQTYVetos);
}

// Avoid "stack too deep" by placing these variables in memory
struct AllocateLQTYMemory {
VoteSnapshot votesSnapshot_;
GlobalState state;
UserState userState;
InitiativeVoteSnapshot votesForInitiativeSnapshot_;
InitiativeState initiativeState;
InitiativeState prevInitiativeState;
Allocation allocation;
}

/// @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
Expand All @@ -720,9 +736,10 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
"Governance: array-length-mismatch"
);

(VoteSnapshot memory votesSnapshot_, GlobalState memory state) = _snapshotVotes();
AllocateLQTYMemory memory vars;
(vars.votesSnapshot_, vars.state) = _snapshotVotes();
uint16 currentEpoch = epoch();
UserState memory userState = userStates[msg.sender];
vars.userState = userStates[msg.sender];

for (uint256 i = 0; i < _initiatives.length; i++) {
address initiative = _initiatives[i];
Expand All @@ -734,11 +751,11 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
// 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);
(vars.votesForInitiativeSnapshot_, vars.initiativeState) = _snapshotVotesForInitiative(initiative);

(InitiativeStatus status,,) =
getInitiativeState(initiative, votesSnapshot_, votesForInitiativeSnapshot_, initiativeState);
(InitiativeStatus status,,) = getInitiativeState(
initiative, vars.votesSnapshot_, vars.votesForInitiativeSnapshot_, vars.initiativeState
);

if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) {
/// @audit You cannot vote on `unregisterable` but a vote may have been there
Expand All @@ -757,36 +774,36 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
// == INITIATIVE 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
vars.prevInitiativeState = InitiativeState(
vars.initiativeState.voteLQTY,
vars.initiativeState.vetoLQTY,
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.initiativeState.averageStakingTimestampVetoLQTY,
vars.initiativeState.lastEpochClaim
);

// update the average staking timestamp for the initiative based on the user's average staking timestamp
initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp(
initiativeState.averageStakingTimestampVoteLQTY,
userState.averageStakingTimestamp,
vars.initiativeState.averageStakingTimestampVoteLQTY = _calculateAverageTimestamp(
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
initiativeState.voteLQTY,
add(initiativeState.voteLQTY, deltaLQTYVotes)
vars.initiativeState.voteLQTY,
add(vars.initiativeState.voteLQTY, deltaLQTYVotes)
);
initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp(
initiativeState.averageStakingTimestampVetoLQTY,
userState.averageStakingTimestamp,
vars.initiativeState.averageStakingTimestampVetoLQTY = _calculateAverageTimestamp(
vars.initiativeState.averageStakingTimestampVetoLQTY,
vars.userState.averageStakingTimestamp,
/// @audit This is wrong unless we enforce a reset on deposit and withdrawal
initiativeState.vetoLQTY,
add(initiativeState.vetoLQTY, deltaLQTYVetos)
vars.initiativeState.vetoLQTY,
add(vars.initiativeState.vetoLQTY, deltaLQTYVetos)
);

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

// update the initiative's state
initiativeStates[initiative] = initiativeState;
initiativeStates[initiative] = vars.initiativeState;

// == GLOBAL STATE == //

Expand All @@ -812,62 +829,63 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
/// @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,
vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
vars.prevInitiativeState.averageStakingTimestampVoteLQTY,
/// @audit We don't have a test that fails when this line is changed
state.countedVoteLQTY,
state.countedVoteLQTY - prevInitiativeState.voteLQTY
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY - vars.prevInitiativeState.voteLQTY
);
assert(state.countedVoteLQTY >= prevInitiativeState.voteLQTY);
assert(vars.state.countedVoteLQTY >= vars.prevInitiativeState.voteLQTY);
/// @audit INVARIANT: Never overflows
state.countedVoteLQTY -= prevInitiativeState.voteLQTY;
vars.state.countedVoteLQTY -= vars.prevInitiativeState.voteLQTY;

state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
state.countedVoteLQTYAverageTimestamp,
initiativeState.averageStakingTimestampVoteLQTY,
state.countedVoteLQTY,
state.countedVoteLQTY + initiativeState.voteLQTY
vars.state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
vars.state.countedVoteLQTYAverageTimestamp,
vars.initiativeState.averageStakingTimestampVoteLQTY,
vars.state.countedVoteLQTY,
vars.state.countedVoteLQTY + vars.initiativeState.voteLQTY
);

state.countedVoteLQTY += initiativeState.voteLQTY;
vars.state.countedVoteLQTY += vars.initiativeState.voteLQTY;
}

// == 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 = currentEpoch;
require(!(allocation.voteLQTY != 0 && allocation.vetoLQTY != 0), "Governance: vote-and-veto");
lqtyAllocatedByUserToInitiative[msg.sender][initiative] = allocation;
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;

// == USER STATE == //

userState.allocatedLQTY = add(userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos);

emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch);
vars.userState.allocatedLQTY = add(vars.userState.allocatedLQTY, deltaLQTYVotes + deltaLQTYVetos);

// Replaces try / catch | Enforces sufficient gas is passed
safeCallWithMinGas(
bool success = safeCallWithMinGas(
initiative,
MIN_GAS_TO_HOOK,
0,
abi.encodeCall(
IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, userState, allocation, initiativeState)
IInitiative.onAfterAllocateLQTY,
(currentEpoch, msg.sender, vars.userState, vars.allocation, vars.initiativeState)
)
);

emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch, success);
}

require(
userState.allocatedLQTY == 0
|| userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))),
vars.userState.allocatedLQTY == 0
|| vars.userState.allocatedLQTY <= uint88(stakingV1.stakes(deriveUserProxyAddress(msg.sender))),
"Governance: insufficient-or-allocated-lqty"
);

globalState = state;
userStates[msg.sender] = userState;
globalState = vars.state;
userStates[msg.sender] = vars.userState;
}

/// @inheritdoc IGovernance
Expand Down Expand Up @@ -909,12 +927,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
Expand Down Expand Up @@ -950,16 +968,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;
}

Expand Down
23 changes: 15 additions & 8 deletions src/interfaces/IGovernance.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,24 @@ 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;
Expand Down
Loading