Liquity v2 ~ Governance (1st audit)

The protocol was accompanied by some specifications that suggested the following:

• “Initiative can be added permissionlessly, requiring the payment of a 100 BOLD fee, and in the following epoch become active for voting. [...] Initiatives failing to be eligible for a claim during four consecutive epochs may be deregistered permissionlessly, requiring reregistration to become eligible for voting again.”

• “Users may split their votes for and veto votes across any number of initiatives. But cannot vote for and veto vote the same Initiative.”

However, neither of these assumptions seems to be followed in the implementation. More specifically:

• The Governance::registerInitiative function requires the requested initiative not to have an active registration in the system which is determined by the non-zero value of registeredInitiatives mapping. Upon registration, this gets the value of the epoch in which the registration happened. However, when unregistering an initiative this value is not reset back to 0 to allow for re-registration effectively disallowing any failed initiative from being registered again for voting which deviates from the described specification.

• The Governance::allocateLQTY function is responsible for voting or vetoing specific initiatives. However, the way it is implemented does not prevent someone from defining a positive _deltaLQTYVotes and a positive _deltaLQTYVetos which effectively means that they can both vote and veto the same initiative contrary to the specification.

Reentrancy guards are a common method for protecting against reentrancy attacks and are used in various functions of the contract (eg depositLQTY, depositLQTYViaPermit, allocateLQTY, etc).

On the other hand, the “Checks Effects Interactions” (CEI) pattern, in which all external calls are performed at the end of the function after all checks and state updates, is also a common way of avoiding reentrancy attacks, without the cost of reentrancy guards. Other functions of the contract opt for this method instead (eg. registerInitiative, unregisterInitiative, etc).

Although both methods can be effective when used exclusively, combining them is not always safe. The reason is that an adversary can:

• First, call a method protected by a guard
• Regain execution control via an external call
• Then call a CEI method, which does not have a guard so it can be called
• Then return in the guarded method which might perform updates after the call, leaving the contract in an invalid state.

We have identified at least one scenario in which this type of attack is applicable to the governance contract:

• The adversary creates and registers a malicious initiative that he controls

• Then calls allocateLQTY for that initiative (which is nonReentrant)

• allocateLQTY calls _snapshotVotes at the start of its execution and stores the resulting state in memory.

  function allocateLQTY(...) external nonReentrant {
      (, GlobalState memory state) = _snapshotVotes();
  }
  

• Later it calls IInitiative(initiative).onAfterAllocateLQTY which transfers control to the malicious initiative

• From there the adversary can call unregisterInitiative for any initiative (not necessarily the malicious one), which is a CEI function not protected by a guard

• unregisterInitiative changes the contract state and returns back to allocateLQTY

• Finally, at the end of its execution, allocateLQTY saves the globalState:

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

• However, state is a memory variable which has not been affected by the nested call, so changes to globalState made by unregisterInitiative will be lost!

This call leaves the contract in an invalid state, in which initiativeStates has been updated for the unregistered contract, but the changes to countedVoteLQTY and countedVoteLQTYAverageTimestamp are lost. Note also that any CEI function can be called by any nonReentrant function, so there could be other problematic reentrancy scenarios, either in the current or in future versions of the contract (eg in a potential change to withdrawLQTY discussed in L1).

For the above reasons, we recommend using reentrancy guards in all functions, to protect against reentrancies in a simple and consistent manner.

In the BribeInitiative contract, the claimBribes function is supposed to provide a way for the users who have voted for an initiative to claim their allocated bribes. However, the function allows the caller to define the _user from whom they want to claim while the final token transfers are done using the msg.sender value.

function claimBribes(address _user, ClaimData[] calldata _claimData)
    external
    returns (uint256 boldAmount, uint256 bribeTokenAmount)
{
    ...

    // Dedaub: There is no connection between _user and msg.sender below
    if (boldAmount != 0) bold.safeTransfer(msg.sender, boldAmount);
    if (bribeTokenAmount != 0)
      bribeToken.safeTransfer(msg.sender, bribeTokenAmount);
}

This introduces a critical vulnerability in which any adversary can call BribeInitiative::claimBribes providing anyone who has allocated their LQTY on that initiative stealing their bribes.

The idea behind bribes is to incentivize the LQTY holders to vote for a specific initiative in exchange for a portion of the total bribe offered by that initiative. This means that to be eligible for claiming the bribes, you have to allocate part of your voting power to that initiative until the end of the current epoch.

However, a specific call path allows anyone to become eligible to claim the bribes without having active votes for that initiative!

The steps are as follows:

• A new initiative is registered and becomes active for voting. The important note here is that it should not have active bribes for that epoch just yet.

• A user allocates their voting power on the initiative by calling Governance::allocateLQTY with a non-zero deltaLQTYVotes and a zero deltaLQTYVetos.

• The BribeInitiative::onAfterAllocateLQTY hook is triggered and since this is the first-ever allocation of that user on that initiative, they will get the first entry added to their lqtyAllocationByUserAtEpoch records equal to deltaLQTYVotes.

• Right after that call, they call the allocateLQTY function again, but now with a deltaLQTYVotes_1 = -deltaLQTYVotes.

• However, this time the onAfterAllocateLQTY hook will be a no-op since all the top-level checks will evaluate to false as there is no active bribe at the moment and mostRecentEpoch is not 0 anymore because the records have been updated by the 1st call to allocateLQTY already.

• At some point during the current epoch, the initiative owner decides to add some bribes to attract more votes.

• Then, after leaving the necessary epochs to pass, the user who had voted and then removed their votes, will be eligible for claiming part of the bribe that was added without them ever supporting the initiative.

This issue is an alternative exploitation of C3 above. The core difference here is that the property of having no active bribes in the current epoch is not required and anyone can exploit this vulnerability at any moment.

The issue lies in how the DoubleLinkedList is defined and how onAfterAllocateLQTY references it. The double-linked list is ordered with the head on the left (next to the null item with id 0) and the tail on the right. Moreover, when a new item is inserted at position 0 it gets added to the tail of the list.

The onAfterAllocateLQTY hook, however, gets the head element of the list expecting it to be the most recent entry and acts based on this element, but at the same time, it inserts items at the tail of the list. As a result, the mostRecentEpoch variable will always get the value of the oldest entry rather than the most recent as it should.

This condition allows anyone to deallocate their LQTY from an Initiative, but still be able to claim their portion of the bribes from the Initiative.

The steps to reproduce this vulnerability are as follows:

• A user allocates their voting power on an initiative with active bribes by calling Governance::allocateLQTY with a non-zero deltaLQTYVotes and a zero deltaLQTYVetos.

• The BribeInitiative::onAfterAllocateLQTY hook is triggered and since this is the first-ever allocation of that user on that initiative, they will get the first entry added to their lqtyAllocationByUserAtEpoch records equal to deltaLQTYVotes.

• Now, contrary to the other scenario in C3, the user lets the epoch to conclude and becomes eligible to claim the first part of the bribes legitimately so far.

• On this new epoch or in some other in the future the initiative owner decides to add some more bribes for attracting votes.

• The user now makes a trivial call to allocateLQTY with both deltaLQTYVotes and deltaLQTYVetos equal to 0 just for updating their records in the BribeInitiative contract and become eligible for claiming part of this new bribe, as the 0 values are a no-op for their allocated votes, but they do trigger an update to the Initiative via the onAfterAllocateLQTY.

• This trivial call adds a new entry at the tail of the user’s double linked list.

• At that point, the user calls the allocateLQTY with a deltaLQTYVotes_1 = -deltaLQTYVotes, so as to remove their votes from that initiative completely.

• The call triggers the hook again, but now the DoubleLinkedList::getHead is used to get the mostRecentEpoch value which, however, returns the timestamp of the first and oldest entry (head) rather than the timestamp of the one that resulted by the trivial call to allocateLQTY described above (tail).

• This means that the execution will enter the branch of the if statement that tries to insert a new entry to the user’s linked list, but since there is already an entry for that epoch in the list (made by the trivial call) and no duplicates are allowed, the execution will revert failing to remove the bribe allocation from the user.

• This revert, however, will have no effect and go unnoticed since the call is enclosed in a try-catch block.

• As a result, the final state is a user with no active votes for the initiative, but with active bribe allocation in the BribeInitiative‘s records which enables them to claim portion of the bribes without having active votes for that initiative.

In the Governance contract, there are two ways in which anyone can invoke specific hooks on initiatives with logic without performing any actual operation on the Governance contract.

More specifically:

• The unregisterInitiative function only requires an initiative to be registered and meet the unregistration conditions. However, since it only partially unregisters an initiative (see P1 for more) it can be called multiple times for the same initiative without actually changing the state.
  
  For example, the only change in the state the first time it is called is that the initiativeStates values get zeroed. However, it remains registered in the system, since the registeredInitiatives mapping is not cleared.
  
  Thus, anyone can re-call the unregistration function again for that particular Initiative. The checks will pass due to the 0 values and the execution will eventually emit again the UnregisterInitiative event, which could cause issues to off-chain components, and also more importantly trigger the onUnregisterInitiative hook on the initiative which could pose significant security risks for that initiative in case it contains logic that adversaries could profitably exploit.
  
  Even though the initiative should also limit and validate the calls coming from Governance on its own, Governance itself violates the reported specifications which suggest that an initiative can be unregistered only once per epoch and under very specific conditions.

• The allocateLQTY, similarly, does not perform sufficient checks to ensure that the onAfterAllocateLQTY hook can only be reached when meaningful operations have taken place.
  
  For example, providing zero _deltaLQTYVotes and _deltaLQTYVetos, an adversary can go through the function without affecting the state at all, but still invoking the hook which could trigger important logic on specific initiatives.

In the Governance contract, when a new initiative is registered it is left initialized with zero values. The only change is in the registeredInitiatives mapping which keeps track of which initiatives are registered or not.

As a result, considering the above, anyone can permissionlessly unregister an initiative that meets the defined criteria. This, nevertheless, is also the case for the just registered initiatives since their state is zeroed which means that anyone can prevent the system from operating and make users lose their 100 BOND registration fee without even getting into the voting process.

The protocol specifies that for an initiative to be eligible for voting it must be registered in the system, having the registrant pay a fee of 100 BOLD tokens, while the initiative becomes activated for voting on the epoch after the one in which it was registered. The current implementation allows anyone to allocate their LQTY (aka. vote) on initiatives that are not even registered.

The allocateLQTY function of the Governance contract, only checks whether the registration epoch is less than the current epoch to determine if the initiative is active. However, this is also true for any unregistered initiative which has a zero value in the registeredInitiatives mapping.

As a result, anyone can call allocateLQTY directly with an arbitrary initiative bypassing both the 100 BOLD fee and the 1-epoch delay. Should this initiative get enough votes to be eligible for claiming, the beneficiary of the initiative can claim their rewards permissionlessly which is against the expected operation of the protocol.

In the Governance contract, the unregisterInitiative function is supposed to unregister an initiative if any of the two conditions are met. The initiative:

• Has failed to be qualified for a claim for at least 4 consecutive epochs

• Has received 3x more veto votes than the minimum qualifying threshold and also the number of its veto votes is greater than the votes for the initiative

The function's first two calls take snapshots of the global vote state (_snapshotVotes) and the initiative’s vote state (_snapshotVotesForInitiative).

However, the latter changes the epoch of the last snapshot assigning as the new value the timestamp of the previous epoch.

function _snapshotVotesForInitiative(
    address _initiative
) internal returns (
    InitiativeVoteSnapshot memory initiativeSnapshot,
    InitiativeState memory initiativeState
) {
    uint16 currentEpoch = epoch();
    initiativeSnapshot = votesForInitiativeSnapshot[_initiative];
    initiativeState = initiativeStates[_initiative];
    if (initiativeSnapshot.forEpoch < currentEpoch - 1) {
        ...
        // Dedaub: The forEpoch field gets updated in memory and storage
        //         before returning
        initiativeSnapshot.forEpoch = currentEpoch - 1;
        votesForInitiativeSnapshot[_initiative] = initiativeSnapshot;
        ...
    }
}

As a result, the check determining whether the initiative has been inactive for 4 consecutive epochs will always fail since the epoch that unregisterInitiative reads will be the updated one rather than the original. This effectively prevents any stale initiative from being unregistered.

function unregisterInitiative(
    address _initiative
) external {
    require(registeredInitiatives[_initiative] != 0,
      "Governance: initiative-not-registered");
    // Dedaub: Contrary to the specification the call to
    //         _snapshotVotesForInitiative blocks all the stale
    //         initiatives from being unregistered since the condition
    //         highlighted below will never be true

    (, GlobalState memory state) = _snapshotVotes();
    (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_,
     InitiativeState memory initiativeState) =
        _snapshotVotesForInitiative(_initiative);

    // An initiative can be unregistered if it has no votes and has been
    // inactive for 'UNREGISTRATION_AFTER_EPOCHS' epochs or if it has
    // received more vetos than votes and the vetos are more than
    // 'UNREGISTRATION_THRESHOLD_FACTOR' times the voting threshold
    require(
        (
            votesForInitiativeSnapshot_.votes == 0
                && votesForInitiativeSnapshot_.forEpoch +
                     UNREGISTRATION_AFTER_EPOCHS < epoch()
        )
            || (
                vetosForInitiative > votesForInitiativeSnapshot_.votes
                    && vetosForInitiative > calculateVotingThreshold() *
                         UNREGISTRATION_THRESHOLD_FACTOR / WAD
            ),
        "Governance: cannot-unregister-initiative"
    );
    ...
}

In the Governance contract, the allocateLQTY function is used to vote for or veto an initiative. In any case, the onAfterAllocateLQTY hook is triggered on the specified initiatives. The protocol allows any entity to be registered as an initiative, including EOAs, custom contracts compatible with the defined interfaces or contracts that inherit from BribeInitiative.

Regarding the latter, the provided implementation of the onAfterAllocateLQTY hook, among other issues (see C3), does not properly handle the case of users vetoing an initiative.

More specifically:

• A user with no previous interactions with that initiative will have a mostRecentEpoch = 0 since no records exist.

• Moreover, the _vetoLQTY will not be 0 since the user is vetoing the initiative.

• This means that the execution will skip the branch which inserts a new entry to the user’s records and will try to deduct any previous allocation of the user from the total accumulated allocation and also remove an entry from the user’s records.

• However, this last operation will fail since the DoubleLinkedList::remove function reverts when trying to remove a non-existing item.

• This revert, though, will go unnoticed because the call to the hook is wrapped with a try/catch block which is not the proper handling of such a case since the initiatives may have defined more logic around the vetoing action which will never be executed due to the misconfiguration on BribeInitiative.

On depositLQTY the user’s averageStakingTimestamp is updated so that the newly staked tokens do not immediately increase the user’s voting power. For instance, if 1 token is staked for 10 days, then 1 more token is added, the averageStakingTimestamp will be updated so that the current balances of 2 tokens appear to be staked for 5 days, resulting in the same voting power.

The opposite calculation, however, does not happen in withdrawLQTY. If 2 tokens are staked for 5 days, and then 1 is withdrawn, the remaining 1 token will still appear to be staked for 5 days, so half of the voting power will be lost. The fact that 1 more token had been staked for the same period will be “forgotten”.

It might be fairer for users to perform the inverse operation, moving averageStakingTimestamp back, so that it appears that the current balance of 1 token has been staked for 10 days so that the previously acquired voting power is not lost.

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 problem to the contract’s operation, it could be useful to allow such funds to be reused.

For instance, one idea could be to add a function reuseBribe(oldEpoch, futureEpoch), that allows to transfer the funds of an oldEpoch (checking that they are not claimable) to a futureEpoch, in order to attract more votes.

In the Governance contract, the withdrawLQTY function calls the unstake function from the caller’s UserProxy. This in turn unstakes up to the requested amount of LQTY from Liquity Staking v1 and returns how many tokens were withdrawn based on the balances of UserProxy for these tokens.

However, both the event emitted by UserProxy::unstake and Governance::withdrawLQTY are susceptible to reporting other values than the real ones. This can happen because anyone can send tokens directly to the caller’s UserProxy address increasing its balance and inflating the withdrawn amounts with amounts that were not really withdrawn. This can pose issues to off-chain components that consume these specific events and act based on their values.

In the Governance contract, the logic of the _calculateAverageTimestamp function could be a bit simplified by extracting out the _newLQTYBalance == 0 which in both branches of the conditional statement returns 0. Hence, for simplicity, you could check this first to also save some gas by avoiding the unnecessary calls to _averageAge.

In the Governance contract, when registering a new initiative it gets initialized with 0-values, but this is already the case. Moreover, currently, there does not seem to be a way to reregister an initiative which could explain the 0-value initialization of the initiatives upon registration.

In the Governance contract, the allocateLQTY function takes three arrays as arguments, but there are no checks to verify that all three of them have the same length which could revert the execution in case there is a mismatch in the number of items they contain.

In the Governance contract, the require statement in the allocateLQTY function that allows only vetoing post the voting cutoff threshold uses equality checks on both sides of the OR condition. However, it is only reachable by the left expression while the execution will never reach the second if the first evaluates to true.

function allocateLQTY(
    address[] calldata _initiatives,
    int176[] calldata _deltaLQTYVotes,
    int176[] calldata _deltaLQTYVetos
) external nonReentrant {
    ...
    for (uint256 i = 0; i < _initiatives.length; i++) {
        ...
        // Dedaub: The equality check on the second condition
        //         will never be reached
        require(
            deltaLQTYVotes <= 0 || deltaLQTYVotes >= 0 &&
              secondsWithinEpoch() <= EPOCH_VOTING_CUTOFF,
          "Governance: epoch-voting-cutoff"
        );
        ...
    }
    ...
}

In the IGovernance interface, the second field of the Configuration struct has a small typo: uint256 reg[i]strationThresholdFactor

In the IBribeInitiative interface, the comment above the ClaimData::prevTotalLQTYAllocationEpoch field is incomplete.

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.

The test passes since we expect the call to unregisterInitiative to revert.

function test_PoC_M1_unregisterInitiative() public {
    vm.startPrank(user);

    address userProxy = governance.deployUserProxy();

    IGovernance.VoteSnapshot memory snapshot =
      IGovernance.VoteSnapshot(1e18, 1);
    vm.store(address(governance), bytes32(uint256(2)),
      bytes32(abi.encode(snapshot)));
    (uint240 votes,) = governance.votesSnapshot();
    assertEq(votes, 1e18);

    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.warp(block.timestamp + 365 days);

    governance.registerInitiative(baseInitiative3);
    uint16 atEpoch = governance.registeredInitiatives(baseInitiative3);
    assertEq(atEpoch, governance.epoch());

    governance.snapshotVotesForInitiative(baseInitiative3);

    vm.warp(block.timestamp + governance.EPOCH_DURATION() * 5 + 1);
    uint16 forEpoch;
    (votes, forEpoch) =
      governance.votesForInitiativeSnapshot(baseInitiative3);
    assertEq(votes, 0);
    assertEq(forEpoch, atEpoch - 1);

    vm.expectRevert("Governance: cannot-unregister-initiative");
    governance.unregisterInitiative(baseInitiative3);

    // (votes, forEpoch) =
    //   governance.votesForInitiativeSnapshot(baseInitiative3);
    // assertEq(votes, 0);
    // assertEq(forEpoch, atEpoch - 1 + 5);

    vm.stopPrank();
}

The test passes, but the revert of the internal call can be observed in the transaction trace which is silently ignored due to the try/catch block.

function test_PoC_M2_silentRevertOnVetoVote() public {

    vm.startPrank(user);

    address userProxy = governance.deployUserProxy();

    lqty.approve(address(userProxy), 1e18);
    governance.depositLQTY(1e18);

    (uint88 allocatedLQTY, ) = governance.userStates(user);
    assertEq(allocatedLQTY, 0);
    (uint88 countedVoteLQTY,) = governance.globalState();
    assertEq(countedVoteLQTY, 0);

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

    vm.warp(block.timestamp + 365 days);
    governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos);

    (allocatedLQTY,) = governance.userStates(user);
    assertEq(allocatedLQTY, 1e18);

    vm.stopPrank();
}