Skip to content

Commit

Permalink
Merge pull request #92 from liquity/todos
Browse files Browse the repository at this point in the history 
chore: Remove TODO's
  • Loading branch information
@bingen
bingen authored Dec 5, 2024
2 parents 078a766 + b495c05 commit ce7e968
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 36 deletions.
2 changes: 0 additions & 2 deletions src/BribeInitiative.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,15 +107,13 @@ contract BribeInitiative is IInitiative, IBribeInitiative {
uint120(governance.EPOCH_START()) + uint120(_epoch) * uint120(governance.EPOCH_DURATION())
) * uint120(TIMESTAMP_PRECISION);

/// @audit User Invariant
assert(totalAverageTimestamp <= scaledEpochEnd);

uint240 totalVotes = governance.lqtyToVotes(totalLQTY, scaledEpochEnd, totalAverageTimestamp);
if (totalVotes != 0) {
(uint88 lqty, uint120 averageTimestamp) = _decodeLQTYAllocation(lqtyAllocation.value);
require(lqty > 0, "BribeInitiative: lqty-allocation-zero");

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

uint240 votes = governance.lqtyToVotes(lqty, scaledEpochEnd, averageTimestamp);
Expand Down
1 change: 0 additions & 1 deletion src/CurveV2GaugeRewards.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ contract CurveV2GaugeRewards is BribeInitiative {
_depositIntoGauge(_bold);
}

// TODO: If this is capped, we may need to donate here, so cap it here as well
function _depositIntoGauge(uint256 amount) internal {
uint256 total = amount + remainder;

Expand Down
39 changes: 9 additions & 30 deletions src/Governance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
/// @inheritdoc IGovernance
function getLatestVotingThreshold() public view returns (uint256) {
uint256 snapshotVotes = votesSnapshot.votes;
/// @audit technically can be out of synch

return calculateVotingThreshold(snapshotVotes);
}
Expand Down Expand Up @@ -466,16 +465,16 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
uint256 upscaledTotalVotes = uint256(_votesSnapshot.votes);

if (upscaledInitiativeVotes > votingTheshold && !(upscaledInitiativeVetos >= upscaledInitiativeVotes)) {
/// @audit 2^208 means we only have 2^48 left
/// 2^208 means we only have 2^48 left
/// Therefore we need to scale the value down by 4 orders of magnitude to make it fit
assert(upscaledInitiativeVotes * 1e14 / (VOTING_THRESHOLD_FACTOR / 1e4) > upscaledTotalVotes);

// 34 times when using 0.03e18 -> 33.3 + 1-> 33 + 1 = 34
uint256 CUSTOM_PRECISION = WAD / VOTING_THRESHOLD_FACTOR + 1;

/// @audit Because of the updated timestamp, we can run into overflows if we multiply by `boldAccrued`
/// We use `CUSTOM_PRECISION` for this reason, a smaller multiplicative value
/// The change SHOULD be safe because we already check for `threshold` before getting into these lines
/// Because of the updated timestamp, we can run into overflows if we multiply by `boldAccrued`
/// We use `CUSTOM_PRECISION` for this reason, a smaller multiplicative value
/// The change SHOULD be safe because we already check for `threshold` before getting into these lines
/// As an alternative, this line could be replaced by https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/FullMath.sol
uint256 claim =
upscaledInitiativeVotes * CUSTOM_PRECISION / upscaledTotalVotes * boldAccrued / CUSTOM_PRECISION;
Expand Down Expand Up @@ -525,7 +524,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

registeredInitiatives[_initiative] = currentEpoch;

/// @audit This ensures that the initiatives has UNREGISTRATION_AFTER_EPOCHS even after the first epoch
/// This ensures that the initiatives has UNREGISTRATION_AFTER_EPOCHS even after the first epoch
initiativeStates[_initiative].lastEpochClaim = currentEpoch - 1;

// Replaces try / catch | Enforces sufficient gas is passed
Expand Down Expand Up @@ -561,7 +560,6 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

// Must be below, else we cannot reset"
// Makes cast safe
/// @audit Check INVARIANT: property_ensure_user_alloc_cannot_dos
assert(alloc.voteLQTY <= uint88(type(int88).max));
assert(alloc.vetoLQTY <= uint88(type(int88).max));

Expand Down Expand Up @@ -706,7 +704,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
);

if (deltaLQTYVotes > 0 || deltaLQTYVetos > 0) {
/// @audit You cannot vote on `unregisterable` but a vote may have been there
/// You cannot vote on `unregisterable` but a vote may have been there
require(
status == InitiativeStatus.SKIP || status == InitiativeStatus.CLAIMABLE
|| status == InitiativeStatus.CLAIMED,
Expand Down Expand Up @@ -734,14 +732,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
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)
);
Expand All @@ -755,37 +751,22 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG

// == 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
/// 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(
Expand Down Expand Up @@ -850,12 +831,10 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
// Remove weight from current state
uint16 currentEpoch = epoch();

/// @audit Invariant: Must only claim once or unregister
// NOTE: Safe to remove | See `check_claim_soundness`
assert(initiativeState.lastEpochClaim < currentEpoch - 1);

// recalculate the average staking timestamp for all counted voting LQTY if the initiative was counted in
/// @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

state.countedVoteLQTYAverageTimestamp = _calculateAverageTimestamp(
Expand Down Expand Up @@ -896,15 +875,15 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, Ownable, IG
return 0;
}

/// @audit INVARIANT: You can only claim for previous epoch
/// INVARIANT: You can only claim for previous epoch
assert(votesSnapshot_.forEpoch == epoch() - 1);

/// All unclaimed rewards are always recycled
/// Invariant `lastEpochClaim` is < epoch() - 1; |
/// If `lastEpochClaim` is older than epoch() - 1 it means the initiative couldn't claim any rewards this epoch
initiativeStates[_initiative].lastEpochClaim = epoch() - 1;

// @audit INVARIANT, because of rounding errors the system can overpay
/// INVARIANT, because of rounding errors the system can overpay
/// We upscale the timestamp to reduce the impact of the loss
/// However this is still possible
uint256 available = bold.balanceOf(address(this));
Expand Down
1 change: 0 additions & 1 deletion src/interfaces/IGovernance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,6 @@ interface IGovernance {
uint88 countedVoteLQTY; // Total LQTY that is included in vote counting
uint120 countedVoteLQTYAverageTimestamp; // Average timestamp: derived initiativeAllocation.averageTimestamp
}
/// TODO: Bold balance? Prob cheaper

/// @notice Returns the user's state
/// @param _user Address of the user
Expand Down
4 changes: 2 additions & 2 deletions src/utils/SafeCallMinGas.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ function hasMinGas(uint256 _minGas, uint256 _reservedGas) view returns (bool) {
function safeCallWithMinGas(address _target, uint256 _gas, uint256 _value, bytes memory _calldata)
returns (bool success)
{
/// @audit This is not necessary
/// But this is basically a worst case estimate of mem exp cost + operations before the call
/// This is not necessary
/// But this is basically a worst case estimate of mem exp cost + operations before the call
require(hasMinGas(_gas, 1_000), "Must have minGas");

// dispatch message to recipient
Expand Down

0 comments on commit ce7e968

Please sign in to comment.