Liquity V2 Governance 1st Audit audit report

ABSTRACT

Dedaub was commissioned to perform a security audit of the Governance protocol of Liquity v2. The codebase was accompanied by the protocol’s expected specifications and invariants along with relevant documentation. However, various important issues were found affecting significantly the protocol and the way the voting power was calculated. Some of the most important issues involve rounding errors that affect the entire protocol’s bookkeeping. Thus, a major refactoring was deemed necessary for the issues to be addressed which was implemented and reviewed in separate audits.

The protocol was also accompanied by a test suite, but with not extensive coverage as some of the issues could have been easily identified by the tests. It is highly recommended to add more unit and invariant tests to ensure that the protocol operates as intended under all circumstances.

SYSTEM OVERVIEW

Liquity’s Modular Initiative-based Governance protocol was designed so that it incentivizes the LQTY holders to be active in the system and also be eligible to earn portions of Liquity’s earned revenue. Namely, 25% of Liquity’s revenue from borrowing activities is routed to the Governance protocol.

The protocol allows arbitrary addresses, called Initiatives, to be registered as beneficiaries and receive part of the accrued revenue if voted. Every Initiative registration is charged with a fixed fee of 100 BOLD tokens and is a permissionless operation. Both EOAs and contracts can become Initiatives.

Users can get voting power by staking their LQTY tokens in Liquity v1 through the Governance. They are then allowed to either vote or veto Initiatives so that these become eligible to receive part of the revenues or be blocked from participating in the reward distribution. The protocol isolates each user’s bookkeeping by deploying a UserProxy contract for each one which is responsible for handling their staked LQTY.

The voting power of the users increases linearly over time. The protocol uses a mechanism with average timestamps to represent an ideal point in time at which the users could have staked and get the voting power they currently have accrued. When users stake more LQTY, their voting power remains the same at the time of staking but it starts accruing faster from that point onwards. As a result, the average timestamp gets moved forward in time to represent the ideal timestamp that if the user had staked the entire currently staked LQTY amount, they would have the same voting power but with a faster accrual rate. On the other hand, when users unstake their LQTY, they immediately lose part of their voting power as the average timestamp remains the same.

Users voting for or vetoing Initiatives allocate part of their LQTY to them. This is accomplished by attributing part of their staked LQTY and average timestamp records to the Initiatives which are then removed and locked on their personal accounting. The Initiatives need to keep track of their votes and vetos separately.

In order for an Initiative to become eligible for rewards it should have received more votes than vetos and their votes be above the maximum of either the 3% of the total positive votes in the system or the minimum votes required for the Initiative to get a payout of at least 500 BOLD tokens.

The Initiatives can also be removed from the system permissionlessly if they have received 3x more vetos than votes or if they are stale (i.e. haven’t become claimable) for more than 4 consecutive epochs.

The protocol provides a base implementation for the Initiatives in the BribeInitiative contract. The Initiatives are allowed to implement custom logic in various hooks that are called by the Governance after every important operation (i.e. registration, vote allocation, unregistration, reward claimimg, etc.). The existing sample hooks are responsible for updating the Initiative’s internal accounting when users update their allocations on that Initiative. The contract also utilizes a mechanism to further incentivize and attract positive votes. Anyone can send to the Initiative bribe tokens which are then distributed to the various voters of the Initiative.

The protocol operates in epochs. Currently, each epoch lasts 7 days. The users are allowed to vote for an Initiative during the first 6 days but on the last day of the epoch (EPOCH_VOTING_CUTOFF period), the users are only allowed to veto the Initiatives. This design prevents last-minute vote allocation by bad-faith actors to Initiatives that are misaligned with Liquity’s ecosystem. The short veto period gives other stakers a chance to veto such bad-faith Initiatives.

In conclusion, the core functionality of the protocol is implemented in the Governance contract which is responsible for keeping track of all the users and Initiatives while also ensuring that bad actors and adversaries cannot manipulate or exploit the system for their own benefit. The BribeInitiative contract provides a base implementation for the Initiatives which they can extend and adapt to their protocol’s needs.

SETTING & CAVEATS

This audit report mainly covers the contracts of the at-the-time private repository liquity/v2-Governance of the Governance Protocol of Liquity v2 at commit [fbd4d69ae3ed0b39345ab28f41f8b72f62fc69e6](https://github.com/liquity/v2-Governance/commit/fbd4d69ae3ed0b39345ab28f41f8b72f62fc69e6).

Two auditors worked on the codebase for 5 days on the following contracts:

src//
├── BribeInitiative.sol
├── CurveV2GaugeRewards.sol
├── Governance.sol
├── UniV4Donations.sol
├── UserProxy.sol
├── UserProxyFactory.sol
└── interfaces/
    └── utils/

The fixes of the issues included in the report were mostly reviewed as part of a separate full re-audit of the codebase which can be found here: Liquity v2 ~ Governance (2nd audit) - Nov 11, 2024.

The audit’s main target is security threats, i.e., what the community understanding would likely call “hacking”, rather than the regular use of the protocol. Functional correctness (i.e. issues in “regular use”) is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

PROTOCOL-LEVEL CONSIDERATIONS

Deviation from the defined specification

Protocol Level Consideration | Status: INFO

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.

VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.

CRITICAL SEVERITY

The mix of protection mechanisms allows reentrancy attacks

Critical | Status: RESOLVED

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.

  ```sol
  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:

  ```sol
      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.

Adversaries can wipe out the bribes allocated to any user

Critical | Status: RESOLVED

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.

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - C2”

Bribes of future epochs can be claimed without voting the initiative when no active bribes in the current epoch exist

Critical | Status: RESOLVED

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.

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - C3”

Bribes can be claimed without voting the initiative even when bribes in the current epoch exist

Critical | Status: RESOLVED

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.

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - C4”

HIGH SEVERITY

Initiative hooks can be reached multiple times without performing any operations

High | Status: RESOLVED

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.

Anyone can abuse the system by unregistering any new initiative

High | Status: RESOLVED

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.

“Note: For this issue to manifest itself we need to assume that the M1 issue is resolved and that the unregisterInitiative function does use the value of the votesForInitiativeSnapshot mapping before it gets changed by _snapshotVotesForInitiative”

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 registration fee can be entirely skipped and anyone can vote for any random unregistered initiative

High | Status: RESOLVED

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.

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - H3”

MEDIUM SEVERITY

Inactive initiatives cannot be unregistered

Medium | Status: RESOLVED

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.

Governance::_snapshotVotesForInitiative:274

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.

Governance::unregisterInitiative:334

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"

    );
    ...
}

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - M1”

The LQTY allocation hook silently reverts when users veto an initiative

Medium | Status: RESOLVED

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.

“Note: Please, also refer to the corresponding PoC below: Proof of Concept - M2”

LOW SEVERITY

Partially withdrawing tokens results in a loss of voting power

Low | Status: ACKNOWLEDGED

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.

“Note: If withdrawLQTY updates userState then a reentrancy guard should be added, otherwise the attack vector of C1 will be applicable”

Unused bribe funds are locked in the contract

Low | Status: ACKNOWLEDGED

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.

OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

Event arguments can be manipulated to report incorrect data

Advisory | Status: INFO

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.

Code simplification

Advisory | Status: RESOLVED

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.

Unclear initializations of initiatives

Advisory | Status: INFO

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.

Missing parameter array length checks

Advisory | Status: RESOLVED

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.

Unreachable check

Advisory | Status: INFO

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.

Governance::allocateLQTY:382

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"

        );
        ...
    }
    ...
}

Typo in variable name

Advisory | Status: RESOLVED

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

Incomplete comment

Advisory | Status: RESOLVED

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

Compiler bugs

Advisory | Status: INFO

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.

DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.

ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub’s auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

APPENDIX

Proof of Concept - C2

 function test_PoC_C2_taintedClaimBribes() public {
     vm.startPrank(user);
     address userProxy = governance.deployUserProxy();
     lqty.approve(address(userProxy), 1e18);
     governance.depositLQTY(1e18);
     vm.stopPrank();

     vm.warp(block.timestamp + 365 days);

     vm.startPrank(lusdHolder);
     lqty.approve(address(bribeInitiative), 1e18);
     lusd.approve(address(bribeInitiative), 1e18);
     bribeInitiative.depositBribe(1e18, 1e18, governance.epoch() + 1);
     vm.stopPrank();

     vm.startPrank(user);

     vm.warp(block.timestamp + 365 days);

     address[] memory initiatives = new address[](1);
     initiatives[0] = address(bribeInitiative);
     int176[] memory deltaVoteLQTY = new int176[](1);
     deltaVoteLQTY[0] = 1e18;
     int176[] memory deltaVetoLQTY = new int176[](1);
     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     vm.stopPrank();

     vm.startPrank(lusdHolder);
     lqty.approve(address(bribeInitiative), 1e18);
     lusd.approve(address(bribeInitiative), 1e18);
     bribeInitiative.depositBribe(1e18, 1e18, governance.epoch() + 1);
     vm.warp(block.timestamp + governance.EPOCH_DURATION());
     vm.warp(block.timestamp + governance.EPOCH_DURATION());
     vm.stopPrank();

     address adversary =
       address(0x1234123412341234123412341234123412341234);

     vm.startPrank(adversary);
     BribeInitiative.ClaimData[] memory epochs =
       new BribeInitiative.ClaimData[](1);
     epochs[0].epoch = governance.epoch() - 1;

     epochs[0].prevLQTYAllocationEpoch = governance.epoch() - 2;

     epochs[0].prevTotalLQTYAllocationEpoch = governance.epoch() - 2;

     (uint256 boldAmount, uint256 bribeTokenAmount) =
       bribeInitiative.claimBribes(user, epochs);
     assertEq(boldAmount, 1e18);
     assertEq(bribeTokenAmount, 1e18);
     vm.stopPrank();
 }

Proof of Concept - C3

 function test_PoC_C3_claimBribeWithNoVotes() public {
     vm.startPrank(user);
     address userProxy = governance.deployUserProxy();
     lqty.approve(address(userProxy), 1e18);
     governance.depositLQTY(1e18);
     vm.stopPrank();

     vm.warp(block.timestamp + 365 days);

     vm.startPrank(user);

     address[] memory initiatives = new address[](1);
     initiatives[0] = address(bribeInitiative);
     int176[] memory deltaVoteLQTY = new int176[](1);
     deltaVoteLQTY[0] = 1e18;
     int176[] memory deltaVetoLQTY = new int176[](1);
     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(
       user, governance.epoch()), 1e18);

     deltaVoteLQTY[0] = -1e18;

     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     (uint88 allocatedLQTY, ) = governance.userStates(user);
     assertEq(allocatedLQTY, 0);
     assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(
       user, governance.epoch()), 1e18);

     vm.stopPrank();

     vm.startPrank(lusdHolder);
     lqty.approve(address(bribeInitiative), 1e18);
     lusd.approve(address(bribeInitiative), 1e18);
     bribeInitiative.depositBribe(1e18, 1e18, governance.epoch() + 1);
     vm.warp(block.timestamp + governance.EPOCH_DURATION());
     vm.warp(block.timestamp + governance.EPOCH_DURATION());
     vm.stopPrank();

     vm.startPrank(user);

     BribeInitiative.ClaimData[] memory epochs =
       new BribeInitiative.ClaimData[](1);
     epochs[0].epoch = governance.epoch() - 1;

     epochs[0].prevLQTYAllocationEpoch = governance.epoch() - 2;

     epochs[0].prevTotalLQTYAllocationEpoch = governance.epoch() - 2;

     (uint256 boldAmount, uint256 bribeTokenAmount) =
       bribeInitiative.claimBribes(user, epochs);
     assertEq(boldAmount, 1e18);
     assertEq(bribeTokenAmount, 1e18);

     vm.stopPrank();
 }

Proof of Concept - C4

 function test_PoC_C4_claimBribeWithNoVotes_advanced() public {
     vm.startPrank(user);
     address userProxy = governance.deployUserProxy();
     lqty.approve(address(userProxy), 1e18);
     governance.depositLQTY(1e18);
     vm.stopPrank();

     vm.warp(block.timestamp + 365 days);

     vm.startPrank(lusdHolder);
     lqty.approve(address(bribeInitiative), 1e18);
     lusd.approve(address(bribeInitiative), 1e18);
     bribeInitiative.depositBribe(1e18, 1e18, governance.epoch() + 1);
     vm.stopPrank();

     vm.startPrank(user);

     address[] memory initiatives = new address[](1);
     initiatives[0] = address(bribeInitiative);
     int176[] memory deltaVoteLQTY = new int176[](1);
     deltaVoteLQTY[0] = 1e18;
     int176[] memory deltaVetoLQTY = new int176[](1);
     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(
       user, governance.epoch()), 1e18);

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

     deltaVoteLQTY[0] = 0;
     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     (uint88 allocatedLQTY, ) = governance.userStates(user);
     assertEq(allocatedLQTY, 1e18);
     assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(
       user, governance.epoch()), 1e18);

     deltaVoteLQTY[0] = -1e18;

     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);

     (allocatedLQTY, ) = governance.userStates(user);
     assertEq(allocatedLQTY, 0);
     assertEq(bribeInitiative.lqtyAllocatedByUserAtEpoch(
       user, governance.epoch()), 1e18);

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

     BribeInitiative.ClaimData[] memory epochs =
       new BribeInitiative.ClaimData[](1);
     epochs[0].epoch = governance.epoch() - 1;

     epochs[0].prevLQTYAllocationEpoch = governance.epoch() - 1;

     epochs[0].prevTotalLQTYAllocationEpoch = governance.epoch() - 1;

     (uint256 boldAmount, uint256 bribeTokenAmount) =
       bribeInitiative.claimBribes(user, epochs);
     assertEq(boldAmount, 1e18);
     assertEq(bribeTokenAmount, 1e18);

     vm.stopPrank();
 }

Proof of Concept - H3

 function test_PoC_H3_allocateAndClaimOnUnregisteredInitiatives()
     public
 {
     address baseInitiative4 = address(
         new BribeInitiative(
             address(vm.computeCreateAddress(address(this),
               vm.getNonce(address(this)) + 1)),
             address(lusd),
             address(lqty)
         )
     );

     vm.startPrank(user);

     // deploy
     address userProxy = governance.deployUserProxy();

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

     vm.warp(block.timestamp + 365 days);

     vm.stopPrank();

     vm.startPrank(lusdHolder);
     lusd.transfer(address(governance), 10000e18);
     vm.stopPrank();

     vm.startPrank(user);

     assertEq(governance.registeredInitiatives(baseInitiative4), 0);

     address[] memory initiatives = new address[](1);
     initiatives[0] = baseInitiative4;
     int176[] memory deltaVoteLQTY = new int176[](1);
     deltaVoteLQTY[0] = 1000e18;
     int176[] memory deltaVetoLQTY = new int176[](1);
     governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
     (uint88 allocatedLQTY,) = governance.userStates(user);
     assertEq(allocatedLQTY, 1000e18);

     vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1);

     assertEq(governance.claimForInitiative(baseInitiative4), 10000e18);
     assertEq(governance.claimForInitiative(baseInitiative4), 0);

     assertEq(lusd.balanceOf(baseInitiative4), 10000e18);

     vm.stopPrank();
 }

Proof of Concept - M1

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();
}

Proof of Concept - M2

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();
}

SUMMARY

ABSTRACT

SYSTEM OVERVIEW

SETTING & CAVEATS

PROTOCOL-LEVEL CONSIDERATIONS

Deviation from the defined specification

VULNERABILITIES & FUNCTIONAL ISSUES

CRITICAL SEVERITY

The mix of protection mechanisms allows reentrancy attacks

Adversaries can wipe out the bribes allocated to any user

Bribes of future epochs can be claimed without voting the initiative when no active bribes in the current epoch exist

Bribes can be claimed without voting the initiative even when bribes in the current epoch exist

HIGH SEVERITY

Initiative hooks can be reached multiple times without performing any operations

Anyone can abuse the system by unregistering any new initiative

The registration fee can be entirely skipped and anyone can vote for any random unregistered initiative

MEDIUM SEVERITY

Inactive initiatives cannot be unregistered

The LQTY allocation hook silently reverts when users veto an initiative

LOW SEVERITY

Partially withdrawing tokens results in a loss of voting power

Unused bribe funds are locked in the contract

OTHER / ADVISORY ISSUES

Event arguments can be manipulated to report incorrect data

Code simplification

Unclear initializations of initiatives

Missing parameter array length checks

Unreachable check

Typo in variable name

Incomplete comment

Compiler bugs

DISCLAIMER

ABOUT DEDAUB

APPENDIX

Proof of Concept - C2

Proof of Concept - C3

Proof of Concept - C4

Proof of Concept - H3

Proof of Concept - M1

Proof of Concept - M2