Livepeer Onchain Treasury Upgrade - Sathish9098's results

GAS OPTIMIZATION

[G-1] Minimizing Storage Overhead by Avoiding Unnecessary Boolean Variables

Saves 20000 GAS

// Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

FILE: 2023-08-livepeer/contracts/treasury/GovernorCountingOverridable.sol

38:   bool hasVoted;

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

[G-2] Using memory can be more gas-efficient when calling the same variable for the second time compared to using storage.

As per gas test this will save around 150 - 200 GAS .

lock.withdrawRound called second time so memory is more gas efficient than storage

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

FILE: 2023-08-livepeer/contracts/bonding/BondingManager.sol

- 251: UnbondingLock storage lock = del.unbondingLocks[_unbondingLockId];
+ 251: UnbondingLock memory lock = del.unbondingLocks[_unbondingLockId];

[G-3] Structs can be packed into fewer storage slots

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct.

lastClaimRound can be uint96 instead of uint256 . Saves 2000 GAS , 1 SLOT

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

uint96 is a data type representing an unsigned integer with 96 bits, which can store values ranging from 0 to 2^96 - 1. While this is a relatively large range of values, whether it's more than enough to store timestamps and durations.

FILE: 2023-08-livepeer/contracts/bonding/BondingManager.sol

58: // Represents a delegator's current state
59:    struct Delegator {
60:        uint256 bondedAmount; // The amount of bonded tokens
61:        uint256 fees; // The amount of fees collected
62:        address delegateAddress; // The address delegated to
+ 65:        uint96 lastClaimRound; // The last round during which the delegator claimed its earnings
63:        uint256 delegatedAmount; // The amount of tokens delegated to the delegator
64:        uint256 startRound; // The round the delegator transitions to bonded phase and is delegated to someone
- 65:        uint256 lastClaimRound; // The last round during which the delegator claimed its earnings
66:        uint256 nextUnbondingLockId; // ID for the next unbonding lock created
67:        mapping(uint256 => UnbondingLock) unbondingLocks; // Mapping of unbonding lock ID => unbonding lock
68:    }

lastClaimRound can be uint96 instead of uint256 . Saves 2000 GAS , 1 SLOT

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

uint96 is a data type representing an unsigned integer with 96 bits, which can store values ranging from 0 to 2^96 - 1. While this is a relatively large range of values, whether it's more than enough to store timestamps and durations.

FILE: 2023-08-livepeer/contracts/bonding/BondingVotes.sol

struct BondingCheckpoint {
        /**
         * @dev The amount of bonded tokens to another delegate as of the lastClaimRound.
         */
        uint256 bondedAmount;
        /**
         * @dev The address of the delegate the account is bonded to. In case of transcoders this is their own address.
         */
        address delegateAddress;
+         uint96 lastClaimRound;
        /**
         * @dev The amount of tokens delegated from delegators to this account. This is only set for transcoders, which
         * have to self-delegate first and then have tokens bonded from other delegators.
         */
        uint256 delegatedAmount;
        /**
         * @dev The last round during which the delegator claimed its earnings. This pegs the value of bondedAmount for
         * rewards calculation in {EarningsPoolLIP36-delegatorCumulativeStakeAndFees}.
         */
-         uint256 lastClaimRound;
        /**
         * @dev The last round during which the checkpointed account called {BondingManager-reward}. This is needed to
         * when calculating pending rewards for a delegator to this transcoder, to find the last earning pool available
         * for a given round. In that case we start from the delegator checkpoint and then fetch its delegate address
         * checkpoint as well to find the last earning pool.
         *
         * Notice that this is the only field that comes from the Transcoder struct in BondingManager, not Delegator.
         */
        uint256 lastRewardRound;
    }

[G-4] IF’s/require() statements that check input arguments should be at the top of the function

FAIL CHEEPLY INSTEAD OF COSTLY

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.

Cheaper to check the function parameter before creating storage variables

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

FILE: 2023-08-livepeer/contracts/bonding/BondingManager.sol

+ 253:        require(isValidUnbondingLock(msg.sender, _unbondingLockId), "invalid unbonding lock ID");
250:  Delegator storage del = delegators[msg.sender];
251:        UnbondingLock storage lock = del.unbondingLocks[_unbondingLockId];
252:
- 253:        require(isValidUnbondingLock(msg.sender, _unbondingLockId), "invalid unbonding lock ID");
254:        require(
255:            lock.withdrawRound <= roundsManager().currentRound(),
266:            "withdraw round must be before or equal to the current round"
267:        );


+  754:        require(_amount > 0, "unbond amount must be greater than 0");
750: require(delegatorStatus(msg.sender) == DelegatorStatus.Bonded, "caller must be bonded");
751:
752:        Delegator storage del = delegators[msg.sender];
753:
- 754:        require(_amount > 0, "unbond amount must be greater than 0");
755:        require(_amount <= del.bondedAmount, "amount is greater than bonded amount");

[G-5] State variables that are used multiple times in a function should be cached in stack variables

Caching state variable with memory saves 100 GAS , 1 SLOD

lock.withdrawRound , del.bondedAmount , del.delegateAddress , t.cumulativeRewards should be cached : Saves 900 GAS , 9 SLOD

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

FILE: 2023-08-livepeer/contracts/bonding/BondingManager.sol

+    uint256 withdrawRound = lock.withdrawRound;
253: require(isValidUnbondingLock(msg.sender, _unbondingLockId), "invalid unbonding lock ID");
        require(
            withdrawRound <= roundsManager().currentRound(),
            "withdraw round must be before or equal to the current round"
        );

        uint256 amount = lock.amount;
-        uint256 withdrawRound = lock.withdrawRound;
        // Delete unbonding lock
        delete del.unbondingLocks[_unbondingLockId];

        // Tell Minter to transfer stake (LPT) to the delegator
        minter().trustedTransferTokens(msg.sender, amount);

        emit WithdrawStake(msg.sender, _unbondingLockId, amount, withdrawRound);

+ uint256 bondedAmount_ = del.bondedAmount ;
- if (del.bondedAmount > 0) {
+ if (bondedAmount_ > 0) {
            uint256 penalty = MathUtils.percOf(delegators[_transcoder].bondedAmount, _slashAmount);

            // If active transcoder, resign it
            if (transcoderPool.contains(_transcoder)) {
                resignTranscoder(_transcoder);
            }

            // Decrease bonded stake
-             del.bondedAmount = del.bondedAmount.sub(penalty);
+             del.bondedAmount = bondedAmount_.sub(penalty);

+  uint256 delegateAddress_ = del.delegateAddress ;
- 415:  delegators[del.delegateAddress].delegatedAmount = delegators[del.delegateAddress].delegatedAmount.sub(
+ 415:  delegators[delegateAddress_ ].delegatedAmount = delegators[delegateAddress_].delegatedAmount.sub(
416:                    penalty
417:                );


+ uint256 cumulativeRewards_ = t.cumulativeRewards;
-  t.activeCumulativeRewards = t.cumulativeRewards;
+  t.activeCumulativeRewards = cumulativeRewards_ ;


        uint256 transcoderCommissionRewards = MathUtils.percOf(_rewards, earningsPool.transcoderRewardCut);
        uint256 delegatorsRewards = _rewards.sub(transcoderCommissionRewards);
        // Calculate the rewards earned by the transcoder's earned rewards
        uint256 transcoderRewardStakeRewards = PreciseMathUtils.percOf(
            delegatorsRewards,
            t.activeCumulativeRewards,
            earningsPool.totalStake
        );
        // Track rewards earned by the transcoder based on its earned rewards and rewardCut
-         t.cumulativeRewards = t.cumulativeRewards.add(transcoderRewardStakeRewards).add(transcoderCommissionRewards);
+         t.cumulativeRewards = cumulativeRewards_ .add(transcoderRewardStakeRewards).add(transcoderCommissionRewards);
   

+  address delegateAddress_ = del.delegateAddress ;
        // Only will have earnings to claim if you have a delegate
        // If not delegated, skip the earnings claim process
-        if (del.delegateAddress != address(0)) {
+        if (delegateAddress_ != address(0)) {
            (currentBondedAmount, currentFees) = pendingStakeAndFees(_delegator, _endRound);

            // Check whether the endEarningsPool is initialised
            // If it is not initialised set it's cumulative factors so that they can be used when a delegator
            // next claims earnings as the start cumulative factors (see delegatorCumulativeStakeAndFees())
-            Transcoder storage t = transcoders[del.delegateAddress];
+            Transcoder storage t = transcoders[delegateAddress_ ];
            EarningsPool.Data storage endEarningsPool = t.earningsPoolPerRound[_endRound];
            if (endEarningsPool.cumulativeRewardFactor == 0) {
                uint256 lastRewardRound = t.lastRewardRound;
                if (lastRewardRound < _endRound) {
                    endEarningsPool.cumulativeRewardFactor = cumulativeFactorsPool(t, lastRewardRound)
                        .cumulativeRewardFactor;
                }
            }
            if (endEarningsPool.cumulativeFeeFactor == 0) {
                uint256 lastFeeRound = t.lastFeeRound;
                if (lastFeeRound < _endRound) {
                    endEarningsPool.cumulativeFeeFactor = cumulativeFactorsPool(t, lastFeeRound).cumulativeFeeFactor;
                }
            }

-            if (del.delegateAddress == _delegator) {
+            if (delegateAddress_  == _delegator) {
                t.cumulativeFees = 0;
                t.cumulativeRewards = 0;
                // activeCumulativeRewards is not cleared here because the next reward() call will set it to cumulativeRewards
            }
        }

        emit EarningsClaimed(
-            del.delegateAddress,
+            delegateAddress_ ,
            _delegator,
            currentBondedAmount.sub(del.bondedAmount),
            currentFees.sub(del.fees),
            startRound,
            _endRound
        );

+  address delegateAddress_ = del.delegateAddress ;
- 1582: increaseTotalStake(del.delegateAddress, amount, _newPosPrev, _newPosNext);
+ 1582: increaseTotalStake(delegateAddress_, amount, _newPosPrev, _newPosNext);

-        emit Rebond(del.delegateAddress, _delegator, _unbondingLockId, amount);
+        emit Rebond(delegateAddress_ , _delegator, _unbondingLockId, amount);

bond.delegateAddress , bond.lastClaimRound , bond.bondedAmount should be cached : Saves 300 GAS , 3 SLOD

https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingVotes.sol#L464-L487

FILE: Breadcrumbs2023-08-livepeer/contracts/bonding/BondingVotes.sol

+ address delegateAddress_ = bond.delegateAddress ;
+ uint256 lastClaimRound_ = bond.lastClaimRound ;
+ uint256 bondedAmount_ = bond.bondedAmount ;
464:  EarningsPool.Data memory startPool = getTranscoderEarningsPoolForRound(
-            bond.delegateAddress,
+            delegateAddress_,
-            bond.lastClaimRound
+            delegateAddress_
        );

        (uint256 rewardRound, EarningsPool.Data memory endPool) = getLastTranscoderRewardsEarningsPool(
-            bond.delegateAddress,
+            delegateAddress_,
            _round
        );

-  if (rewardRound < bond.lastClaimRound) {
+  if (rewardRound < lastClaimRound_ ) {
 // If the transcoder hasn't called reward() since the last time the delegator claimed earnings, there wil be
            // no rewards to add to the delegator's stake so we just return the originally bonded amount.
-            return bond.bondedAmount;
+            return bondedAmount_;
        }

(uint256 stakeWithRewards, ) = EarningsPoolLIP36.delegatorCumulativeStakeAndFees(
            startPool,
            endPool,
-            bond.bondedAmount,
+            bondedAmount_,
            0
        );

[G-6] Don't cache state variables or functions only used once

Saves 20-40 Gas

FILE: Breadcrumbs2023-08-livepeer/contracts/bonding/BondingManager.sol

- 355: uint256 treasuryRewards = MathUtils.percOf(rewards, treasuryRewardCutRate);
- 356:  rewards = rewards.sub(treasuryRewards);
+ 356:  rewards = rewards.sub(MathUtils.percOf(rewards, treasuryRewardCutRate));

- 358: uint256 transcoderCommissionRewards = MathUtils.percOf(rewards, earningsPool.transcoderRewardCut);
- 359: uint256 delegatorsRewards = rewards.sub(transcoderCommissionRewards);
+ 359: uint256 delegatorsRewards = rewards.sub(MathUtils.percOf(rewards, earningsPool.transcoderRewardCut));

- uint256 transcoderCommissionFees = _fees.sub(delegatorsFees);
        // Calculate the fees earned by the transcoder's earned rewards
        uint256 transcoderRewardStakeFees = PreciseMathUtils.percOf(
            delegatorsFees,
            activeCumulativeRewards,
            totalStake
        );
        // Track fees earned by the transcoder based on its earned rewards and feeShare
-        t.cumulativeFees = t.cumulativeFees.add(transcoderRewardStakeFees).add(transcoderCommissionFees);
+        t.cumulativeFees = t.cumulativeFees.add(transcoderRewardStakeFees).add(_fees.sub(delegatorsFees));

- address oldDelDelegate = oldDel.delegateAddress;

        unbondWithHint(_amount, _oldDelegateNewPosPrev, _oldDelegateNewPosNext);

        uint256 oldDelUnbondingLockId = oldDel.nextUnbondingLockId.sub(1);
        uint256 withdrawRound = oldDel.unbondingLocks[oldDelUnbondingLockId].withdrawRound;

        // Burn lock for current owner
        delete oldDel.unbondingLocks[oldDelUnbondingLockId];

        // Create lock for new owner
        uint256 newDelUnbondingLockId = newDel.nextUnbondingLockId;

        newDel.unbondingLocks[newDelUnbondingLockId] = UnbondingLock({ amount: _amount, withdrawRound: withdrawRound });
        newDel.nextUnbondingLockId = newDel.nextUnbondingLockId.add(1);

        emit TransferBond(msg.sender, _delegator, oldDelUnbondingLockId, newDelUnbondingLockId, _amount);

        // Claim earnings for receiver before processing unbonding lock
        uint256 currentRound = roundsManager().currentRound();
        uint256 lastClaimRound = newDel.lastClaimRound;
        if (lastClaimRound < currentRound) {
            updateDelegatorWithEarnings(_delegator, currentRound, lastClaimRound);
        }

        // Rebond lock for new owner
        if (newDel.delegateAddress == address(0) && newDel.bondedAmount == 0) {
            // Requirements for caller
            // Does not trigger self-delegation
            require(oldDelDelegate != _delegator, "INVALID_DELEGATOR");

-             newDel.delegateAddress = oldDelDelegate;
+             newDel.delegateAddress = oldDel.delegateAddress;
        }

- 758: uint256 currentRound = roundsManager().currentRound();
- 759:        uint256 withdrawRound = currentRound.add(unbondingPeriod);
+ 759:        uint256 withdrawRound = roundsManager().currentRound().add(unbondingPeriod);

- 872:   uint256 treasuryBalance = livepeerToken().balanceOf(treasury());
- 873:   if (treasuryBalance >= treasuryBalanceCeiling && nextRoundTreasuryRewardCutRate > 0) {
+ 873:   if (livepeerToken().balanceOf(treasury()) >= treasuryBalanceCeiling && nextRoundTreasuryRewardCutRate > 0) {

- 912: uint256 endRound = roundsManager().currentRound();
- 913: (uint256 stake, ) = pendingStakeAndFees(_delegator, endRound);
+ 913: (uint256 stake, ) = pendingStakeAndFees(_delegator, roundsManager().currentRound());

- 927:  uint256 endRound = roundsManager().currentRound();
- 928:  (, uint256 fees) = pendingStakeAndFees(_delegator, endRound);
+ 928:  (, uint256 fees) = pendingStakeAndFees(_delegator, roundsManager().currentRound());

- 1271: address delegateAddr = del.delegateAddress;
- 1272: bool isTranscoder = _delegator == delegateAddr;
+ 1272: bool isTranscoder = _delegator == del.delegateAddress;

https://github.com/code-423n4/2023-08-livepeer/blob/main/contracts/bonding/BondingManager.sol

Analysis - Livepeer Onchain Treasury Upgrade

Summary

Overview

Decentralized video infrastructure protocol powering video in web3's leading social and media applications

Livepeer is a protocol for developers who want to add live or on-demand video to their project. It aims to increase the reliability of video streaming while reducing costs associated with it by up to 50x.

To achieve this Livepeer is building p2p infrastructure that interacts through a marketplace secured by the Ethereum blockchain.

Who Can Benefit from Livepeer

Developers

Who want to build applications that include live or on demand video can use Livepeer to power their video functionality

Users

Who want to stream video, gaming, coding, entertainment, educational courses, and other types of content can use applications built on Livepeer to do so.

Broadcasters

Such as Twitch who have large audiences and high streaming bills or infrastructure costs can use Livepeer to reduce costs or infrastructure overhead.

How Livepeer Works ?

When Bob opens the app and taps Record at the start of each game, the app sends the live video along with broadcaster fees into the Livepeer network. Livepeer then transcodes the video into all the formats and bitrates that his viewers can consume.

Key actors in Livepeer network

• Orchestrators
• Delegators

ORCHESTRATORS

• In Livepeer, anyone can join the network and become what's known as an orchestrator by running software that allows you to contribute your computer's resources (CPU, GPU, and bandwidth) in service of transcoding and distributing video for paying broadcasters and developers

• For doing so, you earn fees in the form of a cryptocurrency like ETH or a stablecoin pegged to the US dollar like DAI

• In order to earn the right to do this type of work on the network, you must first earn or acquire Livepeer Token, also known as LPT

DELEGATORS

• Delegators are Livepeer tokenholders who participate in the network by staking their tokens towards orchestrators who they believe are doing good and honest work.

LPT Token (Livepeer Token)

The purpose of the Livepeer token (LPT) is to coordinate, bootstrap, and incentivize participants to make sure the Livepeer network is as cheap, effective, secure, reliable and useful as possible.

Rewards Distrubution over ORCHESTRATORS and DELEGATORS

• When a broadcaster pays fees into the network, both orchestrators and Delegators earn a portion of those fees as a reward for ensuring a high-quality and secure network.

• In addition to earning fees, Livepeer mints new token over time, much like Bitcoin and Ethereum block rewards, which are split amongst Delegators and orchestrators in proportion to their total stake relative to others in the network.

Audit approach

I followed below steps while analyzing and auditing the code base.

1. Initial Read and Note-Taking Reading the contest's Readme.md file and took essential notes

• Livepeer Onchain Treasury Upgrade
  • Inheritance
  • 100% Test Suit
  • This an upgrade of an existing system - checkpointing support for an existing staking system, abstract it as an IERC5805 token and set up an OpenZeppelin Governor
  • Uses Timelock and OZ
  • Deployed on L2 but no bridges involved
  • Protocol is already audited
  • Areas to be addressed
    • Find any inconsistency in the stake checkpoint with the actual stake in BondingManager in the corresponding round (as per the LIP-89 spec);
    • Force the vote counting logic to behave differently from the specification, fabricating or losing voting power, altering other users votes (apart from the override), etc;
    • Exploit the governor/treasury into running arbitrary code without a proper proposal;
    • Find any loopholes for the LivepeerGovernor to make changes to the protocol itself;
    • Distort the treasury contribution to something different than what is configured;

2. Analyzed the over all codebase one iterations very fast

3. Study of documentation to understand each contract purposes, its functionality, how it is connected with other contracts, etc.

4. Then i read old audits and already known findings. Then go through the bot races findings

5. Then setup my testing environment things. Used yarn commands to test the protocol

Commands Used for testing:

• yarn compile
• yarn test:audit

6. Finally, I started with the auditing the code base in depth way I started understanding line by line code and took the necessary notes to ask some questions to sponsors.

Stages of audit

• The first stage of the audit

During the initial stage of the audit for Livepeer Onchain Treasury Upgrade, the primary focus was on analyzing GAS usage and Quality assurance (QA) aspects. This phase of the audit aimed to ensure the efficiency of gas consumption and verify the robustness of the platform.

• The second stage of the audit

In the second stage of the audit for Livepeer Onchain Treasury Upgrade, the focus shifted towards understanding the protocol usage in more detail. This involved identifying and analyzing the important contracts and functions within the system. By examining these key components, the audit aimed to gain a comprehensive understanding of the protocol's functionality and potential risks.

• The third stage of the audit

During the third stage of the audit for Livepeer Onchain Treasury Upgrade, the focus was on thoroughly examining and marking any doubtful or vulnerable areas within the protocol. This stage involved conducting comprehensive vulnerability assessments and identifying potential weaknesses in the system. Found 60-70 vulnerable and weakness code parts all marked with @audit tags.

• The fourth stage of the audit

During the fourth stage of the audit for Livepeer Onchain Treasury Upgrade, a comprehensive analysis and testing of the previously identified doubtful and vulnerable areas were conducted. This stage involved diving deeper into these areas, performing in-depth examinations, and subjecting them to rigorous testing, including fuzzing with various inputs. Finally concluded findings after all research's and tests. Then i reported C4 with proper formats

Key Learnings

Livepeer is a blockchain-based protocol tailored for developers seeking to integrate live or on-demand video streaming into their applications. It leverages a decentralized marketplace secured by the Ethereum blockchain to enhance the reliability of video streaming while dramatically reducing associated costs, potentially up to 50 times. Within this ecosystem, Orchestrators play a pivotal role by contributing computing resources (CPU, GPU, and bandwidth) to transcode and distribute video content for paying developers and broadcasters. They receive compensation in the form of cryptocurrency, such as ETH or stablecoins pegged to the US dollar (e.g., DAI). Additionally, the Livepeer token (LPT) serves as a critical incentive mechanism, allowing token holders to perform transcoding work on the network in exchange for fees. Delegators, another crucial participant group, secure the network by staking their LPT tokens with Orchestrators they trust. This system ensures high-quality and secure video streaming experiences. Orchestrators and Delegators, through their active participation, also benefit from token rewards and network ownership growth. The protocol operates in rounds, with each round lasting approximately 22.4 hours, and the inflation rate automatically adjusts based on token staking, further incentivizing participation to maintain network health.

Possible Systemic Risks

Network Congestion and Scalability

High congestion could disrupt Livepeer's video transcoding and streaming operations, potentially resulting in reduced quality of service and higher costs

Orchestrator Reliability

Inconsistent Orchestrator performance can lead to service interruptions and affect the overall quality and reliability of Livepeer's video streaming capabilities

Token Volatility

Significant fluctuations in LPT's value can influence the economic incentives for network participants, potentially affecting the stability and security of the Livepeer ecosystem

Lack of Decentralization

Over-centralization, where a small number of Orchestrators dominate the network, may compromise Livepeer's intended decentralization and security

Centralization risks

Contracts with privileged functions that rely on a single owner or admin for control can introduce several risks

FILE: 2023-08-livepeer/contracts/bonding/BondingManager.sol

155:     function setUnbondingPeriod(uint64 _unbondingPeriod) external onlyControllerOwner {

167:     function setTreasuryRewardCutRate(uint256 _cutRate) external onlyControllerOwner {

176:     function setTreasuryBalanceCeiling(uint256 _ceiling) external onlyControllerOwner {

186:     function setNumActiveTranscoders(uint256 _numActiveTranscoders) external onlyControllerOwner {

• setUnbondingPeriod () The ability to modify the unbonding period is concentrated in the hands of the controller owner. This means that participants must trust this single entity to make responsible decisions regarding the unbonding period. If the controller owner were to act maliciously or make unfavorable changes, it could disrupt the protocol and potentially harm users.

• setTreasuryRewardCutRate() Centralizing control over this parameter means that the controller owner has significant influence over the protocol's economic incentives. Users have no direct say in determining the cut rate, which could affect their rewards. Trust is placed in the controller owner to act in the best interests of the network.

• setTreasuryBalanceCeiling() Centralized control over the treasury balance ceiling means that decisions regarding the maximum amount of funds the protocol can hold are made solely by the controller owner. If this limit is set too high or too low, it can have significant financial implications for the protocol

• setNumActiveTranscoders() The number of active transcoders directly affects the network's performance and decentralization. Centralized control over this parameter means that the controller owner can influence the protocol's centralization level without the need for community consensus

Gas Optimizations

The Livepeer protocol demonstrates several gas optimization opportunities that could enhance the efficiency of its smart contracts and reduce transaction costs. First, the use of the memory keyword instead of storage when accessing variables for the second time has been identified as a cost-saving measure.This optimization has been observed in the context of lock.withdrawRound and similar variables within the protocol. Furthermore, optimizing struct packing by using uint96 instead of uint256 for variables like lastClaimRound in certain data structures offers additional savings. In addition, gas-efficient programming practices recommend placing require statements that check input arguments at the beginning of functions, allowing for early and less costly failures. Lastly, caching state variables used multiple times within a function in stack variables, such as withdrawRound, bondedAmount, and delegateAddress, can lead to significant savings in terms of both GAS and SLOD operations

Code Commentry and Risks

BondingManager.sol

Code Commentry

• In the constructor, consider adding input validation to ensure that the provided _controller address is not zero (address(0)). This will prevent accidental deployment with an invalid controller address
• setUnbondingPeriod, setTreasuryBalanceCeiling, setNumActiveTranscoders, setTreasuryRewardCutRate,setTreasuryRewardCutRate and other similar functions, consider adding validation checks to ensure that the input parameters are within acceptable ranges. For example, ensure that _unbondingPeriod > 0, > MIN and < MAX
• For MAX_FUTURE_ROUND, consider adding a brief explanation of why this value is set to 2**256 - 1. This can help other developers understand the purpose of this constant
• Add visibility modifers for MAX_FUTURE_ROUND constant
• Implement checks-effects-interactions pattern in functions that interact with external contracts. Ensure that state changes occur after external calls to prevent reentrancy vulnerabilities
• In the TranscoderUpdate event, you could include the previous values of rewardCut and feeShare.
• Consider renaming _cutRate in the setTreasuryRewardCutRate function to something like _newCutRate for clarity
• Maintain a consistent naming convention for variables and functions. For example, stick with camelCase also private varibales should start with _ as per solidity naming standard
• In the bondForWithHint and unbondWithHint functions, consider consolidating the logic for changing the delegate and updating the bonded amount into a separate internal function to enhance readability and reduce code duplication
• Ensure that all relevant events are emitted consistently when state changes occur. This allows external applications to listen for and react to these events
• Implement fallback and receive functions for accidential ether send to contract to avoid unexpected behavior
• Add emergency stop functions and functions to recover accidential ether send
• Consider implementing access control mechanisms to restrict certain functions to authorized users or roles. OpenZeppelin's Access Control library is a useful tool for this purpose

Risks

1. The contract relies on an "owner" (Controller owner) to make critical parameter updates, such as setUnbondingPeriod, setTreasuryRewardCutRate, setTreasuryBalanceCeiling, and setNumActiveTranscoders. If the owner's address is compromised or acts maliciously, it could lead to inappropriate changes in protocol parameters
2. The contract uses a loop to update transcoders' states in the reward function. While the loop appears bounded by the number of transcoders, exceeding gas limits can lead to function execution failures
3. Large stakeholders or transcoders with significant stake can potentially manipulate the protocol's rewards or governance decisions
4. In functions like bondForWithHint and transferBond, it's important to consider the order in which transactions are processed, especially when it involves changing delegate addresses or transcoder positions in the pool. Transaction ordering can impact the final state of the contract, potentially leading to unintended behavior
5. If multiple users or contracts interact with your contract concurrently, race conditions might arise.
6. Be aware of the implications of adding or removing transcoders from the pool. This can affect the selection of active transcoders and their rewards. Ensure that the pool remains in a consistent state after each operation

BondingVotes.sol

Code Commentry

• Import only needed things instead of over all contracts
• Instead of using if statements for validation and reverting, consider using the require statement. This makes the code more explicit and easier to read.
• In several places, you check if _round is greater than or equal to clock() + 1. You can consolidate this check into a helper function to reduce code repetition and make it more readable
• In the bondingManager and roundsManager functions, you can use a bytes32 constant to represent the contract name for getContract. This can make the code more readable and consistent
• Replace magic numbers with named constants to improve code readability and maintainability

Risks

1. The clock() function relies on block timestamps to determine the current round. Any manipulation of block timestamps can affect the accuracy of round calculations.
2. Functions with complex calculations and storage operations, such as delegatorCumulativeStakeAt, may be at risk of exceeding the gas limit for Ethereum transactions
3. This risk could manifest in the getBondingCheckpointAt and delegatorCumulativeStakeAt functions, which retrieve and manipulate bonding checkpoint data. If these functions do not accurately reflect the state of the system, data consistency issues may arise
4. The code uses sorting operations in functions like getTotalActiveStakeAt and getBondingCheckpointAt. Sorting algorithm used is inefficient, it can lead to higher gas costs when querying historical data. Consider optimizing sorting algorithms

GovernorCountingOverridable.sol

Code Commentry

• The code imports several contracts and interfaces from OpenZeppelin, such as Initializable, GovernorUpgradeable, and IERC5805Upgradeable. It also imports contracts and libraries specific to the project
• Add comprehensive comments and documentation throughout your code. Explain the purpose of functions, variables, and complex logic. Good documentation makes it easier for other developers (and your future self) to understand and maintain the code
• Consider using function modifiers to enhance code readability and maintainability
• The code defines custom vote types (VoteType) to represent "For," "Against," and "Abstain" votes, aligning with the Governor Bravo's ordering. This helps maintain clarity in the code and the voting process

Risks

1. f the votes() contract is a delegate call to an external contract, there are potential risks associated with delegate calls, such as reentrancy vulnerabilities. Carefully audit the external contract and consider using the latest best practices for secure delegate calls
2. The code imports contracts and libraries from external sources, such as OpenZeppelin. Ensure that these dependencies are up to date and compatible with the specified Solidity version to avoid vulnerabilities from outdated code
3. If this contract is part of a larger system, consider the implications of contract upgrades on governance and voting. Upgrades should not compromise the integrity of past votes or introduce unexpected changes in governance rules
4. This risk could potentially affect the entire contract, especially where timestamps are used for vote delegation and tracking. The relevant code areas include functions like proposalSnapshot, where timestamps are used to determine delegation eligibility
5. The gas cost for tallies can be a concern in functions that tally votes, such as _quorumReached and _voteSucceeded. If there are a large number of proposals or voters, gas costs could accumulate in these functions

LivepeerGovernor.sol

Code Commentry

• Try to reduce the inheritance list
• _controller not checked with address(0)
• initialVotingDelay,initialVotingPeriod,initialProposalThreshold,initialQuorum,quota Lack of checkes for interger values
• IVotes(controller.getContract(keccak256("BondingVotes"))) , Treasury(payable(controller.getContract(keccak256("Treasury")))) save this with constants
• Array length not checked in _execute(),_cancel() functions
• consider adding comments to internal functions as well, especially if they contain complex logic. This helps future developers understand the purpose and behavior of these functions
• While Solidity automatically assigns the internal visibility to functions that don't specify visibility, consider explicitly specifying visibility to improve code readability and clarit

Non-functional aspects

• Aim for high test coverage to validate the contract's behavior and catch potential bugs or vulnerabilities
• The protocol execution flow is not explained in efficient way.
• Its best to explain over all protocol with architecture is easy to understandings
• Consider Using upgradable contracts

Time spent on analysis

15 Hours

Time spent:

15 hours