Livepeer Onchain Treasury Upgrade - twicek's results

Lines of code

https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L273-L278 https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L130-L133 https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1667-L1673 https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1500-L1554

Vulnerability details

Proof of Concept

The withdrawFees function is calling autoClaimEarnings which will update the bondedAmount of the msg.sender delegator's account with new rewards. Here is the execution path:

BondingManager.sol#L273-L278

function withdrawFees(address payable _recipient, uint256 _amount)
        external
        whenSystemNotPaused
        currentRoundInitialized
        autoClaimEarnings(msg.sender)
    {

BondingManager.sol#L130-L133

modifier autoClaimEarnings(address _delegator) {
        _autoClaimEarnings(_delegator);
        _;
    }

BondingManager.sol#L1667-L1673

function _autoClaimEarnings(address _delegator) internal {
        uint256 currentRound = roundsManager().currentRound();
        uint256 lastClaimRound = delegators[_delegator].lastClaimRound;
        if (lastClaimRound < currentRound) {
            updateDelegatorWithEarnings(_delegator, currentRound, lastClaimRound);
        }
    }

(updateDelegatorWithEarnings summarized) BondingManager.sol#L1500-L1554

function updateDelegatorWithEarnings(
        address _delegator,
        uint256 _endRound,
        uint256 _lastClaimRound
    ) internal {
        Delegator storage del = delegators[_delegator];
        uint256 startRound = _lastClaimRound.add(1);
        uint256 currentBondedAmount = del.bondedAmount;
        uint256 currentFees = del.fees;


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

        del.lastClaimRound = _endRound;
        // Rewards are bonded by default
        del.bondedAmount = currentBondedAmount;
        del.fees = currentFees;
    }

As you can see above, updateDelegatorWithEarnings will update the delegator's bondedAmount with currentBondedAmount which is being updated with rewards in pendingStakeAndFees. Any update of a checkpointed field requires that autoCheckpoint is called, which is not done in withdrawFees.

Impact

Voting power determined by the checkpointed bondedAmount will not be up to date with the current bondedAmount which will reduce voting power for delegators.

Tool used

Manual Review

Recommended Mitigation Steps

Consider using the autoCheckpoint modifier in withdrawFees.

function withdrawFees(address payable _recipient, uint256 _amount)
        external
        whenSystemNotPaused
        currentRoundInitialized
        autoClaimEarnings(msg.sender)
	autoCheckpoint(msg.sender)
    {

Assessed type

Other