Liquity v2 ~ Governance (2nd audit)

During the audits, issues with the protocol’s calculations affecting its bookkeeping were discovered. They concern rounding issues in a couple of places with the most important and impactful being in the _calculateAverageTimestamp function which is responsible for moving the users’, the Initiatiaves’ or the global average timestamp back and forth in time so that it properly reflects the virtual point in time in which the entity should have ideally deposited their LQTY such that if multiplied with the new LQTY balance it would result in the voting power they are currently entitled to.

However, in some cases, these calculations were found to introduce significant rounding errors that desync the properties and the invariants which should otherwise apply as long as the protocol operates.

Some representative factors that seem to contribute to the manifestation of these errors, which of course are only a subset of all the possible conditions, are the following:

• The time elapsed since the users deposited their LQTY until they allocate on Initiatives

• The time elapsed between an allocation and a deallocation on the same Initiative

• The amount of LQTY staked by a user or allocated on Initiatives compared to the amounts accumulated by individual Initiatives so far or in total for the global state.

• The order in which the allocations of an Initiative are made, especially when some of them are of minute amounts compared to the total amounts.

All the examples above affect either the correlation between the global state and the summation of the states of all the Initiatives, the state of the individual Initiatives, the users’ voting powers or other properties that combine the states.

The test suite contains examples and cases trying to investigate and possibly mitigate those issues, but the extent of their dependencies and the numerous execution flows with random user inputs make this attempt quite complex and difficult to resolve.

We also conducted some tests on our end trying to investigate more some of the most affected cases. A simple example that resolved one of them was to increase the precision of the division operations in the Governance::_calculateAverageTimestamp function, but, on the other hand, it did not resolve other expressions of the issue that originated from factors like the order of the allocations or time-sensitive events like multiple allocations and deallocations of the same user in the same or different blocks.

It is unclear if these issues can be eliminated entirely, but a lot more testing and investigation is certainly required to first help identify all the possible affected scenarios and then to find the best possible solution or mitigation such that the protocol does not become insolvent or fall into unrecoverable conditions.

The refactoring of the code introduced changes in the way core parts of the code are used as well as differences in the specification that was initially defined which are listed below for visibility:

• The REGISTRATION_WARM_UP_PERIOD constant is no longer used in the Governance contract. However, it is still being initialized in the constructor. The check that used that constant and previously was in the unregisterInitiative function was moved to the new getInitiativeState function which now uses the hardcoded constraint that the warm-up period lasts 1 epoch.

• In the previous version, the unregistered Initiatives could be registered again in the future by depositing the 100 BOLD registration fee. However, in the current version the Initiatives can only be registered once.

• In the current version, bribes are possible to be deposited for the current epoch when in the previous version bribes could be deposited only for future epochs.

The current version introduced the logic of enforcing the reset of the users’ allocations every time they want to update them. In addition to that, in Governance::allocateLQTY checks were added to ensure that each Initiative has been provided only once and no double processing could be done for the same address. However, both the resetting and the reallocation invoke the Initiatives’ onAfterAllocateLQTY hook which means that during a single allocation the hook can be called twice for the same address possibly breaking the implemented checks.

The new resetting mechanism enforces the users to reset all their allocations before updating their allocations. When resetting, the Governance::_allocateLQTY function invokes the onAfterAllocateLQTY hook on the Initiative.

However, if time sensitive Initiatives existed in the future, which may depend on the timestamps at which the users voted or vetoed for their reward or bribe distributions, resetting and reallocating the users’ votes can inherently affect their accounting and make users lose or get more rewards than supposed to.

The existing BribeInitiative template and the example applications (CurveV2Gauge, UniV4Donations and ForwardBribe) are not time sensitive, but since the protocol aims to support any arbitrary contract as an Initiative, such scenarios are not unlikely to exist.

In Governance::allocateLQTY function, the specifications suggest that during the voting cutoff period, no one can vote for an Initiative, but only veto or remove their votes. This was enforced in the new implementation, but the withdrawal of the vetos is still possible during that period. If withdrawing vetos could be interpreted as voting for an Initiative it may be considered as a deviation from the specification.

In the previous version, there was an issue that allowed anyone to unregister any new Initiative right after its registration abusing the system and permissionlessly censoring specific Initiatives from entering the voting process. A REGISTRATION_WARM_UP_PERIOD constant was added in an attempt to prevent immediate unregistrations, but this only prevented them during the warm-up period. After that period ended, the unregistration was possible again due to the uninitialized values of the Initiative’s state. In this version, although new logic was introduced to determine the state in which an Initiative is, the issue still exists.

More specifically, for an Initiative to be marked as UNREGISTERABLE any of the following should apply:

• Be inactive for more than UNREGISTRATION_AFTER_EPOCHS epochs
• Have more vetos than votes and the vetos be above a threshold (3x more than votes)

function getInitiativeState(...) public view returns (...) {
    ...
    // == Unregister Condition == //
    // e.g. if `UNREGISTRATION_AFTER_EPOCHS` is 4, the 4th epoch flip
    // that would result in SKIP, will result in the initiative being
    // `UNREGISTERABLE`
    if (
          (_initiativeState.lastEpochClaim + UNREGISTRATION_AFTER_EPOCHS <
            epoch() - 1)
        ||   _votesForInitiativeSnapshot.vetos >
              _votesForInitiativeSnapshot.votes
          && _votesForInitiativeSnapshot.vetos >
               votingTheshold * UNREGISTRATION_THRESHOLD_FACTOR / WAD
    ) {
        return (InitiativeStatus.UNREGISTERABLE, lastEpochClaim, 0);
    }

    // == Not meeting threshold Condition == //
    return (InitiativeStatus.SKIP, lastEpochClaim, 0);
}

However, the inactivity is determined by the lastEpochClaim which is the epoch in which the Initiative last claimed its rewards indicating that it is not stale. For newly registered Initiatives this value is initialized to 0 which means that if the protocol has operated for more than UNREGISTRATION_AFTER_EPOCHS epochs, then the Initiative becomes immediately UNREGISTERABLE when the warm-up period ends.

A side effect of this issue is that all Initiatives after the warm-up period will immediately become UNREGISTERABLE blocking any allocation on them to be made since the only allowed states for increasing an allocation are: SKIP, CLAIMABLE and CLAIMED.

Considering what was discussed in H1, there is another way that breaks one of the Initiative unregistration invariants. The invariant that breaks is that an Initiative can be unregistered if it has been stale (i.e. in a SKIP state) for more than UNREGISTRATION_AFTER_EPOCHS epochs.

However, in the following scenario a non-stale Initiative can become UNREGISTERABLE:

• Epoch 0: Assume that UNREGISTRATION_AFTER_EPOCHS = 4 and an Initiative has just been registered.

• Epoch 1: The Initiative exits the warm-up period and becomes eligible for voting.

• Epoch 4: UNREGISTRATION_AFTER_EPOCHS - 1 epochs have passed, since the warm-up period ended, with the Initiative not receiving sufficient votes to become CLAIMABLE in any of the previous epochs rendering it stale during that period.

• Epoch 5: In this epoch, it finally receives enough votes to become CLAIMABLE. Shouldn’t it had been voted during that epoch, the Initiative would have become UNREGISTERABLE as expected.

• Epoch 6: In this epoch, the Initiative is CLAIMABLE, but, for any reason, it does not claim its rewards. This means that the rewards will be reused in this epoch and Initiative’s lastEpochClaim will remain 0. Moreover, during that epoch, all voters remove their votes from the Initiative so that it is not longer CLAIMABLE.

• Epoch 7: Now the Initiative can be immediately unregistered since lastEpochClaim + UNREGISTRATION_AFTER_EPOCHS < epoch() - 1 ⇔ 0 + 4 < 7 - 1 ⇔ 4 < 6. However, this breaks the invariant of unregistration since the Initiative was not stale for UNREGISTRATION_AFTER_EPOCHS consecutive epochs as it was CLAIMABLE in the previous epoch.

In the Governance::_allocateLQTY function, there is a race condition which can break the accounting of the Initiatives when deallocating. More specifically, consider the following scenario:

• A user has staked LQTY and has accumulated a voting power $P$ which corresponds to a timestamp $t_{user}$ for their staked amount of $m_{user}$ LQTY.

• They allocate all their votes to an Initiative. For simplicity, assume that this is the only voter of the Initiative and this is the first allocation it has received. This means that at the time of allocation, the Initiative receives the exact same vote power as the user currently has. The Initiative’s timestamp becomes $t_{init} = t_{user}$ and both the user’s and the Initiative’s lines grow linearly with the same rate.

• At some point in the future the user stakes more LQTY which forwards their average timestamp towards block.timestamp ($t_{user_{new}}$), but their voting power at the time of staking remains the same.

• In the same block, the user decides to deallocate almost the entire amount of their votes from the Initiative above, but not all of them so that the timestamps are not zeroed.

• The _allocateLQTY function will try to recalculate the Initiative’s average timestamp and push it back in time so that it reflects the remaining vote power correctly. However, for this operation, it uses the user’s average timestamp which has just been moved forward and is different than the one with which the user allocated their LQTY on the Initiative in the first place.

• As a result, after the deallocation, the Initiative gets a reduced vote power than the one it should have since the line that will be subtracted is: $(m_{user} - x)(t - t_{user_{new}})$ with $t_{user_{new}} > t_{user}$ and $x$ the LQTY that will be left allocated to the Initiative. The correct line to subtract would be: $(m_{user} - x)(t - t_{user})$.

Another issue that appears from a scenario similar to this one can also be found in M4.

When an Initiative gets unregistered users that had previously voted on it are only allowed to fully deallocate their votes from that Initiative.

In an attempt to address accounting issues originating from rounding errors in the average timestamps calculation, the Governance::unregisterInitiative was refactored so that the Initiative’s votes are immediately removed from the global state. Following this change, the _allocateLQTY function was also modified to prevent double removing the Initiative’s allocations from the global state once the users deallocate their votes.

function _allocateLQTY(...) internal {
    ...
    for (uint256 i = 0; i < _initiatives.length; i++) {
        ...
        // Dedaub:
        // This check prevents double removing the Initiative's votes
        // from the global state when the users deallocate their votes
        if (status != InitiativeStatus.DISABLED) {
            state.countedVoteLQTYAverageTimestamp =
              _calculateAverageTimestamp(
                state.countedVoteLQTYAverageTimestamp,
                prevInitiativeState.averageStakingTimestampVoteLQTY,
                state.countedVoteLQTY,
                state.countedVoteLQTY - prevInitiativeState.voteLQTY
              );
            assert(state.countedVoteLQTY >= prevInitiativeState.voteLQTY);
            state.countedVoteLQTY -= prevInitiativeState.voteLQTY;
        }

        // Dedaub:
        // This segment adds back to the global state the Initiative's
        // votes that were previously removed breaking the accounting,
        // with no way to recover, when the Initiative has been
        // previously unregistered
        state.countedVoteLQTYAverageTimestamp =
          _calculateAverageTimestamp(
            state.countedVoteLQTYAverageTimestamp,
            initiativeState.averageStakingTimestampVoteLQTY,
            state.countedVoteLQTY,
            state.countedVoteLQTY + initiativeState.voteLQTY
          );
        state.countedVoteLQTY += initiativeState.voteLQTY;
        ...
    }
    ...
}

The main idea behind the logic of the segment above is to remove the previous allocation of the Initiative and add back the new allocation with the updated values and timestamps. However, the second part of this logic was left out of the condition that prevents an unregistered Initiative from updating the global state leading to unrecoverable accounting issues.

The issue is that for every user who deallocates their LQTY from that unregistered Initiative the remaining LQTY is added back to the global state when it should not. For example:

• Assume that 5 users had allocated to an Initiative that was unregistered. One of them had allocated, say, 100e18 LQTY and the other four 1e18 LQTY each. This makes the Initiative have a total of 104e18 LQTY allocated.

• The 4 with the small allocations start deallocating their LQTY. For each deallocation the remainder of the total LQTY - user's allocation gets re-added to the global state.

• This results in inflating the global votes by: 103 + 102 + 101 + 100 LQTY = 406 LQTY more than supposed with no ability to restore the global state.

• This affects the voting threshold which is used to determine whether a user has sufficient voting power to register a new Initiative and most importantly is used to determine whether an Initiative is eligible for receiving rewards or not.

As can be seen, in the long term this could completely disable the purpose of the protocol which is to distribute accrued BOLD tokens to voted Initiatives.

The ForwardBribe contract was introduced in the current version and it is another implementation of the base BribeInitiative contract. The only function it implements claims the BOLD tokens from Governance, if eligible, and transfers all the available balance of the contract to the prespecified receiver.

function forwardBribe() external {
    governance.claimForInitiative(address(this));
    // Dedaub:
    // This function permissionlessly transfers the entire balance of the
    // Initiative to the predefined receiver without considering the
    // tokens that should be left for the bribes
    uint boldAmount = bold.balanceOf(address(this));
    uint bribeTokenAmount = bribeToken.balanceOf(address(this));

    if (boldAmount != 0) bold.transfer(receiver, boldAmount);
    if (bribeTokenAmount != 0) bribeToken.transfer(
      receiver, bribeTokenAmount);
}

However, this does not account for the tokens that should be left behind to cover the bribes entitled to be given to users who may have voted for that Initiative. The function can be permissionlessly called and could make the contract insolvent and unable to cover the debt to the users.

In the Governance contract, the _calculateAverageTimestamp function adjusts the average timestamps according to the previous and the new balances. However, there is a race condition in which a non-zero voting power gets zeroed after the increment of the LQTY balance of the user/Initiative.

As long as there are no changes in the LQTY balance, the voting power grows linearly and can be expressed by the following line: $m_0 * (t - t_0)$ (with $m_0$ the amount of LQTY and $t_0$ the average timestamp).

When we increase the amount of the allocated LQTY, the line: $m_1 * (t - t_1)$ is added to the original line resulting in the following: $(m_0 + m_1) * (t - t_{new})$ with $t_{new} = \frac{m_0 * t_{0_{age}} + m_1 * t_{1_{age}}}{m_0 + m_1}$ with $t_{x_{age}} = (t - t_x)$. The new amount of LQTY staked starts with 0 voting power which means that in the calculation above: $t_{1_{age}} = 0 \Leftrightarrow t_1 = t = block.timestamp$.

The protocol defines a hard constraint (invariant) which states that the voting power of a user remains the same the moment they stake more LQTY. Their average timestamp is recalculated such that the multiplication with the new increased LQTY amount results in the same voting power. However, this only applies to the moment of staking since every second that passes afterwards increases the voting power faster due to the new increased LQTY amount ($m_0 + m_1$).

Considering the above, the edge case may manifest itself should the _newLQTYBalance become greater than the previous voting power. In such a scenario the new average timestamp will become equal to block.timestamp and will effectively reset the voting power to 0 even though a non-zero voting power previously existed breaking the invariant.

function _calculateAverageTimestamp(
    uint32 _prevOuterAverageTimestamp,
    uint32 _newInnerAverageTimestamp,
    uint88 _prevLQTYBalance,
    uint88 _newLQTYBalance
) internal view returns (uint32) {
    if (_newLQTYBalance == 0) return 0;
    // prevOuterAverageAge = t0_age = (t - t0)
    uint32 prevOuterAverageAge =
      _averageAge(uint32(block.timestamp), _prevOuterAverageTimestamp);
    // newInnerAverageAge = t1_age = (t - t1)
    uint32 newInnerAverageAge =
      _averageAge(uint32(block.timestamp), _newInnerAverageTimestamp);

    uint88 newOuterAverageAge;
    if (_prevLQTYBalance <= _newLQTYBalance) {
        // deltaLQTY = m1
        uint88 deltaLQTY = _newLQTYBalance - _prevLQTYBalance;
        // prevVotes = m0 * t0_age
        uint240 prevVotes =
          uint240(_prevLQTYBalance) * uint240(prevOuterAverageAge);
        // newVotes = m1 * t1_age
        uint240 newVotes =
          uint240(deltaLQTY) * uint240(newInnerAverageAge);
        uint240 votes = prevVotes + newVotes;
        // newOuterAverageAge = m0 * t0_age + m1 * t1_age / (m0 + m1)
        newOuterAverageAge = uint32(votes / uint240(_newLQTYBalance));
    } else {
        ...
    }

    if (newOuterAverageAge > block.timestamp) return 0;
    return uint32(block.timestamp - newOuterAverageAge);
}

For the example we consider the scenario were a user has staked some LQTY at $t_0$ and at $t_1 > t_0$ stakes more LQTY.

Notations:

• $t_{x_{age}} = (t - t_x)$ $(1)$
• $t_{1_{age}} = (t - t_1) = 0$ (applies the moment the user staked LQTY as the new amount added starts with a zero voting power which means that $t_1 = t = block.timestamp$) $(2)$

The conditions that need to apply for this edge case to occur are as follows:

• $m_0 * t_{0_{age}} + m_1 * t_{1_{age}} < m_0 + m_1 \Leftrightarrow (1)$
• $m_0 * t_{0_{age}} < m_0 + m_1 \Leftrightarrow (2)$
• $m_0 * (t - t_0) < m_0 + m_1 \Leftrightarrow$
• $m_0 * (t - t_0) < m_0 + m_1 \Leftrightarrow$
• $m_1 > m_0 * (t - t_0) - m_0 \Leftrightarrow$
• $m_1 > m_0 * (t - t_0 - 1) \Leftrightarrow$
• $m_1 > m_0 * \text{seconds passed since previous stake}$

As can be seen, since each second that passes requires $m_0$ more LQTY to be staked for this edge case to manifest itself, the scenario could be considered of low likelihood but it is still a race condition that could cause accounting issues and break the voting power invariant.

For voting to be fair, it is important that both positive and negative votes have similar costs. Apart from the cost in terms of voting power obtained by staking, we should also consider the actual gas cost of executing the voting transaction. However, the gas cost can be manipulated by a malicious user to make vetos substantially more expensive than votes, exploiting two aspects of the voting design:

• Every call to allocateLQTY calls the onAfterAllocateLQTY which is controlled by the adversary.

• allocateLQTY requires resetting all previous votes and re-casting them, which essentially means that onAfterAllocateLQTY will be called $2 * N + 1$ times for every voting operation, where $N$ is the number of previously voted Initiatives.

• Now consider a malicious user submitting Initiatives whose onAfterAllocateLQTY consumes all available gas in case of a veto, but a tiny amount of gas in case of a vote. Even though the gas is capped by MIN_GAS_TO_HOOK, the current value of 350.000 is large enough to allow a substantial manipulation. With a gas cost of 20 Gwei (not uncommon), calling the hook alone currently costs around $18. There is a scenario in which an adversary can submit $M$ such Initiatives and make the total hook calls when casting vetos for all of them to be $\sum_{N=0}^{M-1} 2 * N + 1$ which is equal to $M^2$. With 10 malicious Initiatives we need $1800 just to call the hooks (not counting the cost of allocateLQTY itself).

• The idea is that the adversary submits the Initiatives on different epochs and votes for them so that they become eligible to get rewards or just stay active in the system for as long as required (for simplicity we do not consider the warm-up periods and similar mechanisms, as they do not directly affect the scenario).

• On epoch_a, the first malicious Initiative becomes active for voting and the users veto it paying the expensive hook call once.

• On epoch_b > epoch_a the adversary’s 2nd Initiative becomes active and the users veto this as well, but also keep their vetos active for the 1st malicious Initiative. Thus, they pay they will have to pay 2 + 1 times the expensive hooks since they have to reset all their allocations (1 call) and reallocate them back (1 call veto of the 1st Initiative) along with their new allocations (1 call veto for the new (2nd) Initiative).

• As can be seen, each new Initiative requires $2 * (M - 1) + 1$ calls to the expensive hooks resulting in the quadratic cost.

• Since 1 Initiative becomes active every epoch, otherwise if all of them were in the same epoch the users could veto them at once and avoid the quadratic cost, the complexity could be also expressed in epochs and be $O(E^2)$ where $E$ is the number of epochs passed.

The direct manipulation of the vetos could be deemed as the most important, but other expressions of the issue also exist that could be used to manipulate specific execution paths. For example:

• Users who have voted for an Initiative that turned out to be malicious or compromised could experience the same issue. Malicious Initiatives detecting vote reduction could similarly treat it as vetoing and consume all the available gas making the hooks expensive.

• Similarly, every resetting operation that even temporarily zeros all the votes before the reallocation could also be used to abuse the system. Any voted malicious Initiative could make any resetting operation quite expensive.

• As a side effect, the vote removal from unregistered Initiatives (as described in L2) could also be used to make the deallocation unfair and expensive for the users who wish to remove their votes from the inactive Initiative.

In the Governance contract, every time an epoch changes, calls to _snapshotVotes update the global state including the boldAccrued which stores the amount of BOLD tokens to be distributed among the Initiatives that became CLAIMABLE in the previous epoch.

function _snapshotVotes() internal returns (
    VoteSnapshot memory snapshot,
    GlobalState memory state
) {
    bool shouldUpdate;
    (snapshot, state, shouldUpdate) = getTotalVotesAndState();

    if (shouldUpdate) {
        votesSnapshot = snapshot;
        uint256 boldBalance = bold.balanceOf(address(this));
        boldAccrued = (boldBalance < MIN_ACCRUAL) ? 0 : boldBalance;
        emit SnapshotVotes(snapshot.votes, snapshot.forEpoch);
    }
}

However, if we assume that the very first operation of the current epoch is an Initiative registration, the BOLD tokens paid as registration fees will be used by _snapshotVotes and will be considered as part of the rewards for the previous epoch. This happens because, inside the registerInitiative function, the fees are transferred to the contract before the snapshots are taken.

function registerInitiative(
    address _initiative
) external nonReentrant {
    // Dedaub:
    // The registration fee is transferred before accruing the BOLD tokens
    // for the current epoch
    bold.safeTransferFrom(msg.sender, address(this), REGISTRATION_FEE);

    require(_initiative != address(0), "Governance: zero-address");

    // Dedaub:
    // This function calls _snapshotVotes which updates the accrued BOLD
    // based on the current balance of the contract
    (InitiativeStatus status,,) = getInitiativeState(_initiative);

    require(status == InitiativeStatus.NONEXISTENT,
      "Governance: initiative-already-registered");

    address userProxyAddress = deriveUserProxyAddress(msg.sender);
    (VoteSnapshot memory snapshot,) = _snapshotVotes();
    UserState memory userState = userStates[msg.sender];
    ...
}

The snapshots should always be the first operation of functions that update the state to ensure that the previous epoch has been properly concluded before any updates for the current epoch are applied.

For a user to be able to register a new Initiative, they need to pay a flat fee of 100 BOLD tokens and have a voting power greater than a percentage of the total positive (“YES”) votes that all the existing Initiatives have gathered. This was done to prevent system abuse by spamming Initiative registrations as the Governance::registerInitiative function is permissionless.

However, even if a user has sufficient voting power to register an Initiative, there exists a race condition in which they can be blocked from registering following the scenario below:

• The user has staked enough LQTY in a previous epoch such that their voting power before the start of the current epoch surpasses the registration threshold.

• In the running epoch, the user stakes an amount of LQTY that makes their average timestamp greater than the epochStart(), the timestamp at which the current epoch started.

• Then in the same block, the user tries registering a new Initiative. Following the invariant that when a user stakes more LQTY their voting power at that exact moment remains the same, the user should still be able to perform the registration operation.

• However, this does not happen as the voting power calculation uses the epochStart().

• Since epochStart() < user's average timestamp, the user’s voting power will become 0 reverting the registration operation.

function registerInitiative(address _initiative) external nonReentrant {
    ...
    // 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
    require(
      lqtyToVotes(
        uint88(stakingV1.stakes(userProxyAddress)),
        epochStart(),
        userState.averageStakingTimestamp
      )
      >= snapshot.votes * REGISTRATION_THRESHOLD_FACTOR / WAD,
        "Governance: insufficient-lqty"
    );
    ...
}

In the previous version, the require statement above used the block.timestamp, but this was changed to epochStart() as part of the recent changes.

As discussed in M2, the new logic of resetting all previously voted Initiatives in allocateLQTY can lead to a large amount of consumed gas, especially given that the onAfterAllocateLQTY hooks can be controlled by the adversary. The amount of gas is in fact unbounded, since there is no bound on the number of simultaneously voted Initiatives.

This behavior can lead to a gas-based DoS situation if the gas needed to reset a vote is larger than that of allocating the vote. Although this is unlikely to happen under normal operation, a maliciously constructed onAfterAllocateLQTY can consume more gas in case of deallocations (see also M2 for a discussion on malicious gas usage of hooks).

If a user votes for several such Initiatives, they could reach a state in which they have successfully allocated votes for these Initiatives, but the gas needed to reset them surpasses Ethereum’s 30 million block gas limit. This means that the votes will be impossible to reset, and since resetting all initiatives is obligatory in each call, no further calls to allocateLQTY will be possible. Moreover, since the user will be unable to deallocate, all their allocated funds will remain locked and impossible to be unstaked.

To prevent such catastrophic scenarios, we strongly recommend that all operations become able to be performed with a bounded amount of gas. For instance, the contract could provide a reset-only function that allows resetting the votes of a single Initiative, without the constraint that all initiatives must be reset. This will allow users to recover from any gas-based issue by resetting all Initiatives one at a time in separate transactions.

In BribeInitiative, the claimBribes function was adjusted to help mitigate rounding errors in the average timestamp calculations from Governance. The last user to claim their bribes may be eligible for more rewards than the existing funds in the contract due to those errors and the function was made so that it adjusts the amount to be given according to the available balance expecting that only the user’s funds will be available.

function claimBribes(
    ClaimData[] calldata _claimData
) external returns (uint256 boldAmount, uint256 bribeTokenAmount) {
    for (uint256 i = 0; i < _claimData.length; i++) {
        ...
        (uint256 boldAmount_, uint256 bribeTokenAmount_) =
          _claimBribe(...);
        boldAmount += boldAmount_;
        bribeTokenAmount += bribeTokenAmount_;
    }

    if (boldAmount != 0) {
        // Dedaub:
        // This adjustment is not sufficient to prevent the last user
        // from claiming the excessive amount of rewards that may
        // originate from the rounding errors of Governance as more
        // BOLD tokens may exist in the contract

        uint256 max = bold.balanceOf(address(this));
        if (boldAmount > max) {
            boldAmount = max;
        }
        bold.safeTransfer(msg.sender, boldAmount);
    }
    if (bribeTokenAmount != 0) {
        uint256 max = bribeToken.balanceOf(address(this));
        if (bribeTokenAmount > max) {
            bribeTokenAmount = max;
        }
        bribeToken.safeTransfer(msg.sender, bribeTokenAmount);
    }
}

However, this assumption is not always valid as anyone could have deposited more tokens for future bribes inflating the available balance. Moreover, the Initiative could have also claimed their rewards from Governance which would have increased the BOLD balance. As a result, the user’s amount will not be adjusted down as it should, but the amount with the rounding errors will be transferred consuming funds originating from other sources.

In BribeInitiative::_claimBribe function, there are two require statements that revert if the total allocated LQTY or the user’s allocation is 0 blocking any claimings.

function _claimBribe(...) internal returns (...) {
    ...
    DoubleLinkedList.Item memory lqtyAllocation =
      lqtyAllocationByUserAtEpoch[_user].getItem(
        _prevLQTYAllocationEpoch
      );

    require(
         lqtyAllocation.value != 0
      && _prevLQTYAllocationEpoch <= _epoch
      && (lqtyAllocation.next > _epoch || lqtyAllocation.next == 0),
        "BribeInitiative: invalid-prev-lqty-allocation-epoch"
    );
    DoubleLinkedList.Item memory totalLQTYAllocation =
      totalLQTYAllocationByEpoch.getItem(_prevTotalLQTYAllocationEpoch);
    require(
         totalLQTYAllocation.value != 0
      && _prevTotalLQTYAllocationEpoch <= _epoch
      && (totalLQTYAllocation.next > _epoch ||
          totalLQTYAllocation.next == 0),
        "BribeInitiative: invalid-prev-total-lqty-allocation-epoch"
    );
    ...
}

Even though this check worked in the previous version, it is no longer valid in the current code, because the value field now packs more items in the same slot which means that the above checks are performed on the packed value. More specifically, the new code packs the allocated LQTY amount along with the timestamp in the same uint224 variable when previously the LQTY amount was only stored in that field.

function _setTotalLQTYAllocationByEpoch(
    uint16 _epoch, uint88 _lqty, uint32 _averageTimestamp, bool _insert
) private {
    uint224 value = (uint224(_lqty) << 32) | _averageTimestamp;
    if (_insert) {
        totalLQTYAllocationByEpoch.insert(_epoch, value, 0);
    } else {
        totalLQTYAllocationByEpoch.items[_epoch].value = value;
    }
    emit ModifyTotalLQTYAllocation(_epoch, _lqty, _averageTimestamp);
}

Currently, when the new LQTY balance of an entity is zeroed, its average timestamp gets zeroed too which means that, most probably, the packed value will also be 0 when the LQTY amount is 0. However, if this changes in future versions a scenario in which the LQTY amount is 0, but the timestamp is not could exist breaking the assumption and allowing the checks to pass when they should not.

Moreover, this difference does not produce direct issues to the code, as the 0 LQTY amounts will results in 0 products later in the execution, but it should be considered for future changes in that part as well that could be affected by it.

When an Initiative gets unregistered it can still have active votes and vetos allocated to it which can be reset later. However, when the users deallocate their LQTY, the Initiative’s onAfterAllocateLQTY hook gets invoked even when this has been unregistered.

The existing Initiative implementations are not directly affected from this behavior, but one might have expected that once an Initiative gets DISABLED no more updates would have been expected to be forwarded from Governance. This could cause issues to Initiatives that depend on their registration status in Governance for their accounting in future versions.

We raise this issue here also for visibility in case this was not the intended behavior for unregistered Initiatives.

There are two cases in which some of the bribe tokens could end up stuck in the BribeInitiative contract. More specifically:

• In the BribeInitiative::_claimBribe function, the calculation of the bribe shares for a user is susceptible to rounding errors. For example, if the totalVotes do not divide exactly the user’s votes or the bribe token amount, then the remainder will be left in the contract since the resulting amount is rounded down. These tokens are not reused in the next epochs nor does a way to extract them exist which makes them locked in the contract.

• If the epoch of a bribe passes with no allocations, the bribe’s funds will not be claimable by anyone, and at the same time there is no way to recover them, so they will remain locked in the contract. Although this does not pose any direct issues to the contract’s operation, ways to recover such funds could exist to rescue them in such a scenario.

As discussed in M5, the new functionality around updating Initiative allocations can be proven gas-intensive and even lead to unrecoverable situations in edge cases. However, apart from any possible refactoring that could help resolve that issue, some possible ways that could help save some gas are the following:

• A separate function that allows users allocate LQTY on new Initiatives could be added:

• The logic of resetting the allocations only to recast the votes back was mostly introduced for the cases in which users wanted to increase or decrease an existing allocation. However, for new Initiatives there is no need to reset all the others before allocating and such a function could help decouple the different execution paths.

• Similarly, a separate function for fully deallocating from Initiatives is also important to exist as it could help mitigating the issues described in M5. It also could allow for more straightforward deallocations without the need to completely deallocate from all the Initiatives and then allocate back those desired to remain active.

• Moreover, the reset operations are bundled with several other unnecessary for the deallocation operations and storage readings that otherwise are important when updating the allocations.

• The allocateLQTY array parameters _initiativesToReset and _initiatives are checked for duplicate values using a double loop look up. However, you could expect them to be given sorted, reduce the complexity to $O(n)$ and the gas consumption.

• The array length checks inside Governance::_allocateLQTY are not necessary as the top-level calls from allocateLQTY have already checked or enforced the same requirement.

In Governance, the allocateLQTY function was refactored. Currently, the only meaningful values for the _absoluteLQTYVotes and _absoluteLQTYVetos are positive amounts of LQTY to be allocated to Initiatives. This is because all allocations are cleared before any (re)allocation.

As a result, these two parameters should be of type uint88 instead of int88 to avoid the confusion during pure deallocations. When fully deallocating, users need to provide all the arrays empty except from the _initiativesToReset, compared to the previous version in which a deallaction was expecting a negative value to be provided.

In Governance, the getInitiativeState function is used to determine the state of an Initiative. However, there are some possible gas optimizations that could be implemented:

• The first three checks reference the same value in storage all reading it separately. You could cache the value of registeredInitiatives instead to save 2 SLOADs per call.

• The lastEpochClaim value is extracted from the storage mapping initiativeStates but the same information also exists in the _initiativeState parameter which is given as an argument to the function. All the functions that call the getInitiativeState have previously copied the values from the same mapping and they pass them to the function. Thus, you could save 1 SLOAD by using the value of the argument like in the UNREGISTERABLE check.

In Governance::registerInitiative function, there are several parts that could be simplified and save gas on operations that are not necessary for that functionality. For example, in order to check whether the Initiative exists or not the value of registeredInitiatives is sufficient. However, the call to getInitiativeState makes unnecessary calls to _snapshotVotesForInitiative even though the Initiative starts with an empty state. The important factor here is that old Initiatives cannot be re-registered.

In addition to that, getInitiativeState also calls _snapshotVotes which updates the global state. However, the same call is also performed by registerInitiative so that it uses the return values from memory. This means that the storage slots are read twice with no additional benefit for the operations performed.

In Governance::getInitiativeState function, the check for the CLAIMED state uses the 0-initialized return variable in the return clause, but there is no need for that as no value is assigned to it. You could return directly 0 like in all the other similar checks which would also make the code more specific on the expected return values.

In the new version of the codebase, several adjustments were made in the specification of the protocol. One of them is the requirement for the users to reset all their allocated LQTY before allocating more or deallocating. A note above the Governance::_resetInitiatives function suggests that the ResetInitiativeData is desired to be populated only when secondsWithinEpoch() > EPOCH_VOTING_CUTOFF. However, this does not seem to be followed by the code as the resetting is made on every call to allocateLQTY at any point during an epoch.

In Governance::unregisterInitiative function, new checks were added to follow the new model of the Initiatives’ states. An Initiative can be unregistered if its state becomes UNREGISTERABLE. The relevant require statement to enforce this exists, but there are also two more redundant require statements using other possible states of the Initiative. They could have been added for custom error handling or for testing purposes, but we mention it just for visibility.

In Governance::_allocateLQTY, there is a comment which suggests that an Initiative can be voted or vetoed when it is in SKIP, CLAIMABLE, CLAIMED or UNREGISTERABLE state. However, an UNREGISTERABLE Initiative cannot be voted or vetoed and only complete deallocations are allowed.

The code is compiled with Solidity ^0.8.24. For deployment, we recommend no floating pragmas, but a specific version, to be confident about the baseline guarantees offered by the compiler. Version 0.8.24, in particular, has no known bugs at the time of writing.

In this PoC, the following scenario has been followed:

• user2 stakes $m$ LQTY at the beginning of the first epoch and gets an average timestamp equal to the current block.timestamp.

• user stakes $m$ LQTY (the same amount as user2) after half of the epoch has passed.

• After the other half of the epoch, the two users both have $m$ staked LQTY, but the average age of user ($t_0$) is double the average age of user2 ($t_1$ with $t_1 = 2 * t_0$). The average age is the distance of the users’ average timestamps from the current block.timestamp.

• Both user and user2 allocate their $m$ LQTY to the Initiative which gets the following state:

• $\text{InitiativeState.voteLQTY} = 2m$

• $\text{Initiative's Average Age} = \frac{m * t_0 + m * t_1}{2m} = \frac{m * t_0 + m * 2 * t_0}{2m} = \frac{3}{2}t_0$

• user deallocates from the Initiative all their LQTY and the Initiative state becomes:

• $\text{InitiativeState.voteLQTY} = m$

• $\text{Initiative's Average Age} = \frac{2m * \frac{3}{2}t_0 - m * t_0}{m} = \frac{3m * t_0 - m * t_0}{m} = 2t_0$

• user stakes another $m$ LQTY making their average age: $t_{0_{new}} = \frac{t_0}{2}$

• user deallocates from the Initiative all their LQTY and the Initiative state becomes:

• $\text{InitiativeState.voteLQTY} = m$

• $\text{Initiative's Average Age} = \frac{2m * \frac{3}{2}t_0 - m * \frac{t_0}{2}}{m} = \frac{3m * t_0 - m * \frac{t_0}{2}}{m} = \frac{5}{2}t_0$

As can be seen, the only difference in the two scenarios was that user staked more LQTY before deallocating, but the results on the Initiative are different breaking its accounting.

The code snippets below should all be put inside Governance.t.sol in order for the main tests to run. Both the invalid and the valid tests pass, but the values on the invalid are different which indicates the issues described in H3.

function test_PoC_H3_validDeallocation() public {

    (,uint32 userAvt, uint32 user2Avt,,) = _test_PoC_H3_setUp();

    _test_PoC_H3_deallocate();

    (,IGovernance.InitiativeState memory initiativeState,) =
      governance.getInitiativeSnapshotAndState(baseInitiative1);
    assertEq(initiativeState.voteLQTY, 1e18);
    assertEq(
      block.timestamp - initiativeState.averageStakingTimestampVoteLQTY,
      block.timestamp - user2Avt
    );
    assertEq(
      block.timestamp - initiativeState.averageStakingTimestampVoteLQTY,
      2 * (block.timestamp - userAvt)
    );
}

function test_PoC_H3_invalidDeallocation() public {

    (address userProxy, uint32 userAvt,,uint240 userVP,) =
      _test_PoC_H3_setUp();

    // -------------------------------------------------------------------

    vm.startPrank(user);
    lusd.approve(address(governance), 1e18);
    lqty.approve(address(userProxy), 1e18);
    governance.depositLQTY(1e18);

    (, uint32 newUserAvt) = governance.userStates(user);
    uint240 newUserVP =
      governance.lqtyToVotes(
        uint88(governance.stakingV1().stakes(userProxy)
      ),
      governance.epochStart(),
      newUserAvt
    );
    vm.stopPrank();

    assertEq(
      block.timestamp - newUserAvt, (block.timestamp - userAvt) / 2);
    assertEq(newUserVP, userVP);
    // -------------------------------------------------------------------

    _test_PoC_H3_deallocate();

    (,IGovernance.InitiativeState memory initiativeState,) =
      governance.getInitiativeSnapshotAndState(baseInitiative1);
    assertEq(initiativeState.voteLQTY, 1e18);
    assertEq(
      block.timestamp - initiativeState.averageStakingTimestampVoteLQTY,
      5 * (block.timestamp - userAvt) / 2
    );
}

function _test_PoC_H3_helper_deallocate() internal {
    address[] memory removeInitiatives = new address[](2);
    removeInitiatives[0] = baseInitiative1;
    address[] memory newInitiatives = new address[](1);
    int88[] memory removeDeltaLQTYVotes = new int88[](1);
    int88[] memory removeDeltaLQTYVetos = new int88[](1);

    vm.startPrank(user);
    governance.allocateLQTY(
      removeInitiatives, newInitiatives,
      removeDeltaLQTYVotes, removeDeltaLQTYVetos
    );
    vm.stopPrank();

    (uint256 userAllLQTY,) = governance.userStates(user);
    assertEq(userAllLQTY, 0);
}

function test_PoC_H3_setUp() internal returns(
    address userProxy, uint32 userAvt, uint32 user2Avt,
    uint240 userVP, uint240 user2VP
) {

    // -------------------------------------------------------------------

    vm.startPrank(user2);
    address user2Proxy = governance.deployUserProxy();
    vm.stopPrank();

    vm.startPrank(lusdHolder);
    lusd.transfer(user2, 1e18);
    vm.stopPrank();

    vm.startPrank(user2);
    lusd.approve(address(governance), 1e18);
    lqty.approve(address(user2Proxy), 1e18);
    governance.depositLQTY(1e18);
    vm.stopPrank();

    // -------------------------------------------------------------------

    vm.warp(block.timestamp + governance.EPOCH_DURATION() / 2);

    vm.startPrank(user);
    userProxy = governance.deployUserProxy();
    vm.stopPrank();

    vm.startPrank(lusdHolder);
    lusd.transfer(user, 1e18);
    vm.stopPrank();

    vm.startPrank(user);
    lusd.approve(address(governance), 1e18);
    lqty.approve(address(userProxy), 1e18);
    governance.depositLQTY(1e18);
    vm.stopPrank();

    // -------------------------------------------------------------------

    vm.warp(block.timestamp + governance.EPOCH_DURATION() / 2);

    vm.startPrank(user);
    (, userAvt) = governance.userStates(user);
    userVP =
      governance.lqtyToVotes(
        uint88(governance.stakingV1().stakes(userProxy)
      ),
      governance.epochStart(),
      userAvt
    );
    vm.stopPrank();

    vm.startPrank(user2);
    (, user2Avt) = governance.userStates(user2);
    user2VP =
      governance.lqtyToVotes(
        uint88(governance.stakingV1().stakes(user2Proxy)
      ),
      governance.epochStart(),
      user2Avt
    );
    vm.stopPrank();

    assertEq(user2VP, 2 * userVP);
    assertEq(block.timestamp - user2Avt, 2 * (block.timestamp - userAvt));

    // -------------------------------------------------------------------

    address[] memory initiatives = new address[](1);
    initiatives[0] = baseInitiative1;
    int88[] memory deltaLQTYVotes = new int88[](1);
    deltaLQTYVotes[0] = 1e18;
    int88[] memory deltaLQTYVetos = new int88[](1);

    vm.startPrank(user);
    governance.allocateLQTY(
      initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos);
    vm.stopPrank();

    {
        (uint256 userAllLQTY,) = governance.userStates(user);
        assertEq(userAllLQTY, 1e18);

        (,IGovernance.InitiativeState memory initiativeState,) =
          governance.getInitiativeSnapshotAndState(baseInitiative1);
        assertEq(initiativeState.voteLQTY, 1e18);
        assertEq(
          initiativeState.averageStakingTimestampVoteLQTY, userAvt);
    }

    // -------------------------------------------------------------------

    vm.startPrank(user2);
    governance.allocateLQTY(
      initiatives, initiatives, deltaLQTYVotes, deltaLQTYVetos);
    vm.stopPrank();

    (uint256 user2AllLQTY,) = governance.userStates(user2);
    assertEq(user2AllLQTY, 1e18);

    (,IGovernance.InitiativeState memory initiativeState,) =
      governance.getInitiativeSnapshotAndState(baseInitiative1);
    assertEq(initiativeState.voteLQTY, 2e18);
    assertEq(
      block.timestamp - initiativeState.averageStakingTimestampVoteLQTY,
      3 * (block.timestamp - userAvt) / 2
    );
}