Livepeer Onchain Treasury Upgrade - Banditx0x's results

Lines of code

https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/treasury/GovernorCountingOverridable.sol#L174-L212

Vulnerability details

Impact

A delegate can subtract their own voting weight from the voting choice of another delegate, even if that user isn't a transcoder. Since they are not a transcoder, they don't have their votes initially increased by the amount delegated to them, voting weight is still subtracted from the tally of their vote choice.

Maliciously, this could be used to effectively double one's voting power, by delegating their votes to a delegator who is about to vote for the choice which they don't want. It can also occur accidentally, for example when somebody delegates to a transcoder who later switches role to delegate.

Proof of Concept

When a user is not a transcoder, their votes are determined by the amount they have delegated to the delegatedAddress, and does not increase when a user delegates to them:

        if (bond.bondedAmount == 0) {
            amount = 0;
        } else if (isTranscoder) {
            amount = bond.delegatedAmount;
        } else {
            amount = delegatorCumulativeStakeAt(bond, _round);
        }
    }

Lets that this delegator (Alice) has 100 votes and votes For, Then another delegator(Bob) has delegated 1000 votes to Alice As stated above, Alice doesn't get the voting power of Bob's 1000 votes, so the For count increases by 100.

Bob now votes, and _handleVotesOverrides is called. In this function, the first conditional, if isTranscoder will return false as Bob is not self-delegating.

Then, there is a check that the address Bob has delegated to has voted. Note that there is a missing check of whether the delegate address is a transcoder. Therefore the logic inside if (delegateVoter.hasVoted) is executed:

'''solidity if (delegateVoter.hasVoted) { // this is a delegator overriding its delegated transcoder vote, // we need to update the current totals to move the weight of // the delegator vote to the right outcome. VoteType delegateSupport = delegateVoter.support;

        if (delegateSupport == VoteType.Against) {
            _tally.againstVotes -= _weight;
        } else if (delegateSupport == VoteType.For) {
            _tally.forVotes -= _weight;
        } else {
            assert(delegateSupport == VoteType.Abstain);
            _tally.abstainVotes -= _weight;
        }
    }

'''

The logic reduces the tally of whatever choice Alice voted for by Bob's weight. Alice initially voted For with 100 votes, and then the For votes is reduced by Bob's 1000 votes. Lets say that Bob votes Against. This will result in an aggregate 900 vote reduction in the For tally and +1000 votes for Agaisnt after Alice and Bob has finished voting.

If Alice was a transcoder, Bob will be simply reversing the votes they had delegated to Alice. However since Alice was a delegate, they never gained the voting power that was delegated to her.

Bob has effectively gained the ability to vote against somebody else's votes (without first actually increasing their voting power since they are not a transcoder) and can vote themselves, which allows them to manipulate governance.

Tools Used

Manual Review

Recommended Mitigation Steps

There should be a check that a delegate is a transcoder before subtracting the tally. Here is some pseudocode:

if (delegateVoter.hasVoted && ---delegate is transcoder ---)

This is an edit of the conditional of the function _handleOverrides. This ensures that the subtraction of vote tally is only performed when the delegate is a voter AND the delegate is a transcoder. This should fix the accounting/subtraction issue of vote tally for non-transcoder delegates.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L447-L457

Vulnerability details

Impact

Claim Rounds claims pool token shares for the whole round rather than until the end round that the function caller specifies.

Proof of Concept

The claimEarnings function takes in an _endRound parameter. However, this parameter does not affect the round until which the rewards are claimed. Instead, rewards are claimed until the current round.

    function claimEarnings(uint256 _endRound)
        external
        whenSystemNotPaused
        currentRoundInitialized
        autoCheckpoint(msg.sender)
    {

        _endRound;

        _autoClaimEarnings(msg.sender);
    }

This does not match the code comments which clearly state that the tokens should be claimed until the end round:

@notice Claim token pools shares for a delegator from its lastClaimRound through the end round

Tools Used

Manual Review

Recommended Mitigation Steps

Use the _endRound parameter within the claimEarnings function and set it as the upper bound until which rewards are claimed.

Assessed type

Other

Audit approach

My audit approach focused on:

• Vote manipulation
• Checkpointing for all important state changes
• Unexpected inputs in any of the external non-accessed contracts in BondingManager.sol

I paid special attention to the code which was updated in the latest pull request, such as the BondingVotes contract.

I found vulnerabilities in all the above categories.

One mental mode I used for the code was based the "Assets, Actors and Actions" model from Secureum. Below are some notes I collected on Actors and the Actions they can take. Actors are in bold and actions are in dot points in the codebase Analysis:

Codebase Analysis

Transcoders:

• Can vote on behalf of delegate
• Change RewardFee and RewardCut
• Collect Rewards

Rounds Manager:

• initialize round
• set the round length of current and future rounds
• Set lock. The lock restricts when transcoder can change their rewardFee and rewardCut

Verifier:

• Slash Transcoder

Delegator:

• Delegate tokens to transcoder to get share of their rewards
• Unbond and rebond
• Claim rewards that have been collected by the transcoder they are delegated to

Any User:

• Become Transcoder
• Become Delegator
• Bond for another unbonded address. If they increase the amount bonded they have to transfer to mintr.

Assets:

The relevant assets in this audit are votes and tokens. Although votes are not technically an asset, incorrect flow of assets can result in security vulnerabilities.

What is new with the update:

Checkpointing

• Records information to determine voting power
• Checkpoints delegate/transcoder information
• Checkpoints totalActiveStake

The logic for checkpointing is in the BondingVotes.sol contract. These checkpoints are then collected in the functions in BondingManager.sol, mainly using the modifier autoCheckpoint. This modifier runs after the function logic has executed, which makes sure that the checkpoint records the state after all state changes have occurred.

Treasury Fees

• This is deducted during when transcoder calls fees.
• When treasury fees reach the ceiling, the fees are reduced to zero for the next round.

Centralisation and Systemic Risks

• L2Migrator can change delegator's delegateAddress
• Verifier can slash transcoder's stake

How much time did you spend?

I spent around 40 hours on this audit

Time spent:

40 hours