Livepeer Onchain Treasury Upgrade - ladboy233's results

141345 marked the issue as sufficient quality report

There is in fact an issue with the inconsistency with the isRegisteredTranscodeer function. This report didn't manage to go specifically into that issue, but still pointed a valid problem which is in a sense the root cause there. FTR There is no problem with isActiveTranscoder though, since we don't give voting power only to active transcoders. That part of this report is invalid.

victorges (sponsor) confirmed

HickupHH3 marked the issue as selected for report

HickupHH3 marked the issue as satisfactory

This is true that the code counts users that self delegate but didn't join the transcoder set as transcoders, but it does not lead to voting power being calculated incorrectly. _voter.deductions will be 0 for a failed transcoder calculating the _weight correctly. I might be missing something, so correct me if I'm wrong, but if the above is true this finding should be low severity.

This is true that the code counts users that self delegate but didn't join the transcoder set as transcoders, but it does not lead to voting power being calculated incorrectly. _voter.deductions will be 0 for a failed transcoder calculating the _weight correctly. I might be missing something, so correct me if I'm wrong, but if the above is true this finding should be low severity.

Hello, Please check the PR fix 625

https://github.com/livepeer/protocol/pull/625

This is true that the code counts users that self delegate but didn't join the transcoder set as transcoders, but it does not lead to voting power being calculated incorrectly. _voter.deductions will be 0 for a failed transcoder calculating the _weight correctly. I might be missing something, so correct me if I'm wrong, but if the above is true this finding should be low severity.

Here

https://github.com/livepeer/protocol/pull/625/files#diff-def676c8b96f1c8a1d4524f86cf9a2de8a5322052ef804ff46fec780d457600dR402

This is true that the code counts users that self delegate but didn't join the transcoder set as transcoders, but it does not lead to voting power being calculated incorrectly. _voter.deductions will be 0 for a failed transcoder calculating the _weight correctly. I might be missing something, so correct me if I'm wrong, but if the above is true this finding should be low severity.

Please check the PR fix 625

livepeer/protocol#625

Thanks for the context but I believe this PR is for #194 and #96. The comments at the end https://github.com/livepeer/protocol/pull/625#issuecomment-1731448211 confirm my reasoning.

My question is, how does voting power get affected by your issue? If you can find a scenario that is not shown in #194 and #96 that alters voting power then I agree that this should be a unique medium severity.

Thanks for the comment

I feel like twicek's comment is the same as:

This report didn't manage to go specifically into that issue, but still pointed a valid problem which is in a sense the root cause there

Delegating to an account that is not actually a transcoder should render no voting power

in the function delegatorCumulativeStakeAt

the fix make sure when transcoder is not registered, Delegating to unregistered an account return 0

which is used the function getBondingStateAt

this function is in fact used to calculate the vote amount

emm user does relies on this function to get the correct voting power,

I think the inconsitency root from the function getVotes and the actual vote power override

fix related discussion is here

the sponsor has been act in good faith so I think the report does add value by make sure the protocol handle the voting power calculation correctedly when a address is not an registered transcoder so a medium severity is well justified :)

Ok, it makes sense. Thanks for taking the time to explain your reasoning. Good job @JeffCX !

141345 marked the issue as sufficient quality report

victorges marked the issue as disagree with severity

Slashing is turned off in the protocol, so this specific issue is not that important. It did point towards an issue in BondingVotes tho in the way that the getBondingState function checks if transcoder is a transcoder or not and defaults to 0 when bondedAmount is 0. There are other ways that this logic can be triggered (not through slashTranscoder) that can show inconsistencies as well, so I'm flagging this as valid for the underlying issue that it hints to, even though it does not describe a realistic scenario about how this issue could happen.

victorges (sponsor) confirmed

I found the POC hard to follow. A coded instance would have tremendously helped, but what I gather is:

• fully slashed transcoder that votes will do so with 0 weight
• any delegator that delegated their votes to this specific transcoder intending to change their vote may face an issue with sub overflow because _handleVoteOverrides() uses the slashed transcoder's bondedAmount instead of the 0 weight. so this issue seems valid, although its premise is that the slashing functionality is in place.

The README did not indicate that slashing was turned off, though I see it's mentioned a couple of times in the Discord channel. Should've been added in the "Known caveats / limitations" section.

Hence, IMO it isn't entirely fair to wardens if I invalidate their issue because they weren't aware of this fact.

Leaning towards a downgrade to M because of the external requirement of the active slashing mechanism, but am open to discussion.

HickupHH3 changed the severity to 2 (Med Risk)

HickupHH3 marked the issue as primary issue

HickupHH3 marked the issue as selected for report

HickupHH3 marked the issue as satisfactory

Revisiting this issue, IMO it's unreasonable to expect the sponsor to explicitly list out all the gotchas / limitations of the protocol. Some will only pop into mind through discussions with wardens, which seemed to be the case here when the slashing deprecation was mentioned a couple of times in the Discord channel.

On the flip side, the fact remains that the slashing mechanism is part of the audit scope, and was counted towards the sLOC, plus the points I raised above. There wasn't an indication in the code that slashing had been deprecated.

It's a difficult decision here, considering the different parties' POV. There could be some wardens that didn't submit issues related to slashing because of what was brought up in the Discord channel, and those that did (as one can see referenced above) because they didn't monitor the channel.

141345 marked the issue as sufficient quality report

Not sure if we'll make this change, but it is indeed a valid report as well!

victorges (sponsor) acknowledged

HickupHH3 marked the issue as selected for report

HickupHH3 marked the issue as satisfactory

@HickupHH3 re-reviewing this, I think this should be QA as well

the reason why I left it at M is because of the edge case that the treasury gets hacked, but I assume it's a timelock contract, so the probability is very low, & there isn't as many expected calls to the treasury as compared to the BondingManager.

downgrading to QA (L) then.

HickupHH3 changed the severity to QA (Quality Assurance)

1L

victorges (sponsor) disputed

@HickupHH3 as I was looking into a mitigation for this, I realized that the report is actually invalid.

The GovernorTimelockUpgradeable extension already provides an updateTimelock function, which can only be called by governance proposals and updates the address of the timelock (Treasury): https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/release-v4.9/contracts/governance/extensions/GovernorTimelockControlUpgradeable.sol#L163

So this is actually not an issue.

HickupHH3 marked the issue as grade-c

Sorry I should add more detail in my original report:

The admin of the livepeer contract is call setContractInfo

    function setContractInfo(
        bytes32 _id,
        address _contractAddress,
        bytes20 _gitCommitHash
    ) external override onlyOwner {
        registry[_id].contractAddress = _contractAddress;
        registry[_id].gitCommitHash = _gitCommitHash;

        emit SetContractInfo(_id, _contractAddress, _gitCommitHash);
    }

and then other contract read contract address by calling get contract

function getContract(bytes32 _id) public view override returns (address) {
	return registry[_id].contractAddress;
}

if we take a look at the bondManager contract, every time the code tries to retrieve the treasury contract here and here, the code read the storage data from the controller directedly

so even the treasury address change, the updated address is still used in the BondManager

function treasury() internal view returns (address) {
	return controller.getContract(keccak256("Treasury"));
}

but if the admin call setContractInfo to update treasury address

the external integration that calls LivepeerGovernor.sol#treasury is getting the updated treasury address

    function treasury() internal view returns (Treasury) {
        return Treasury(payable(controller.getContract(keccak256("Treasury"))));
    }

but in fact, the old / outdated is still used in the governance contract

the old treasury is init here in this line of code

 __GovernorTimelockControl_init(treasury());

which calls this line of code to set the timelock contract

the timelock contract is used to determine the state of proposal, and incorrect return the state of proposal heavily break external integration (user may think a proposal pass but it is not)

it is true the admin can update the timelock contract by executing this code

    /**
     * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates
     * must be proposed, scheduled, and executed through governance proposals.
     *
     * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
     */
    function updateTimelock(TimelockControllerUpgradeable newTimelock) external virtual onlyGovernance {
        _updateTimelock(newTimelock);
    }

but it is not as easy as calling setContractInfo

the core issue is we can say calling updateTimelock when updating the timelock contract, does not call setContractInfo to update the treasury address,

we can say calling setContractInfo does not update the timelock contract, but either way, the timelock contract address and the treasury address can out of sync

the precondition is indeed admin calls setContractInfo directly to update treasury without care,

but I think given that the treasury address is used to determine the proposal state and there maybe external integration that read the treasury address from governance contract (read the timelock contract directedly)

as for comments

the reason why I left it at M is because of the edge case that the treasury gets hacked

emm the consider that the treausry address is not ugpradeable, maybe the livepeer governance wants to build a new feature on treasury address,

it can cause change of treasury address for dev or techical reason as well (just like the projects want to add additional feature like reward cut and vote override)

emm this bug report submission, based on the comment above, help sponsor realize that the timelock contract can be changed by executing governance proposal updateTimelock (apology I would have add this point in original submission)

and in other words, this bug report, if not found, the likehood that admin directedly call setContractInfo to update treasury contract and not changing timelock contract is high

I politely think that a medium severity is ok

I fully respect judge's expertise and respect judge's final decision

Sorry I should add more detail in my original report:

The admin of the livepeer contract is call setContractInfo

    function setContractInfo(
        bytes32 _id,
        address _contractAddress,
        bytes20 _gitCommitHash
    ) external override onlyOwner {
        registry[_id].contractAddress = _contractAddress;
        registry[_id].gitCommitHash = _gitCommitHash;

        emit SetContractInfo(_id, _contractAddress, _gitCommitHash);
    }

and then other contract read contract address by calling get contract

function getContract(bytes32 _id) public view override returns (address) {
	return registry[_id].contractAddress;
}

if we take a look at the bondManager contract, every time the code tries to retrieve the treasury contract here and here, the code read the storage data from the controller directedly

so even the treasury address change, the updated address is still used in the BondManager

function treasury() internal view returns (address) {
	return controller.getContract(keccak256("Treasury"));
}

but if the admin call setContractInfo to update treasury address

the external integration that calls LivepeerGovernor.sol#treasury is getting the updated treasury address

    function treasury() internal view returns (Treasury) {
        return Treasury(payable(controller.getContract(keccak256("Treasury"))));
    }

but in fact, the old / outdated is still used in the governance contract

the old treasury is init here in this line of code

 __GovernorTimelockControl_init(treasury());

which calls this line of code to set the timelock contract

the timelock contract is used to determine the state of proposal,

    function state(uint256 proposalId) public view virtual override(IGovernorUpgradeable, GovernorUpgradeable) returns (ProposalState) {
        ProposalState currentState = super.state(proposalId);

        if (currentState != ProposalState.Succeeded) {
            return currentState;
        }

        // core tracks execution, so we just have to check if successful proposal have been queued.
        bytes32 queueid = _timelockIds[proposalId];
        if (queueid == bytes32(0)) {
            return currentState;
        } else if (_timelock.isOperationDone(queueid)) {
            return ProposalState.Executed;
        } else if (_timelock.isOperationPending(queueid)) {
            return ProposalState.Queued;
        } else {
            return ProposalState.Canceled;
        }
    }

and incorrect return the state of proposal heavily break external integration (user may think a proposal pass but it is not)

it is true the admin can update the timelock contract by executing this code

    /**
     * @dev Public endpoint to update the underlying timelock instance. Restricted to the timelock itself, so updates
     * must be proposed, scheduled, and executed through governance proposals.
     *
     * CAUTION: It is not recommended to change the timelock while there are other queued governance proposals.
     */
    function updateTimelock(TimelockControllerUpgradeable newTimelock) external virtual onlyGovernance {
        _updateTimelock(newTimelock);
    }

but it is not as easy as calling setContractInfo

the core issue is we can say calling updateTimelock when updating the timelock contract, does not call setContractInfo to update the treasury address,

we can say calling setContractInfo does not update the timelock contract, but either way, the timelock contract address and the treasury address can out of sync

the precondition is indeed admin calls setContractInfo directly to update treasury without care,

but I think given that the treasury address is used to determine the proposal state and there maybe external integration that read the treasury address from governance contract (read the timelock contract directedly)

as for comments

the reason why I left it at M is because of the edge case that the treasury gets hacked

emm the consider that the treausry address is not ugpradeable, maybe the livepeer governance wants to build a new feature on treasury address,

it can cause change of treasury address for dev or techical reason as well (just like the projects want to add additional feature like reward cut and vote override)

emm this bug report submission, based on the comment above, help sponsor realize that the timelock contract can be changed by executing governance proposal updateTimelock (apology I would have add this point in original submission)

and in other words, this bug report, if not found, the likehood that admin directedly call setContractInfo to update treasury contract and not changing timelock contract is high

I politely think that a medium severity is ok

I fully respect judge's expertise and respect judge's final decision

@JeffCX Those are some fair points. I think this can be summarized as "multiple sources of truth for treasury address":

• Primary: the one in LivepeerGovernor, used for everything else (including the riskier proposal interations)
• Duplicate: The one in Controller, only used by BondingManager to make treasury contributions [1]

I agree that the treasury() function on the LivepeerGovernor can be misleading though, since it does not return the actual timelock being used by the governor in case the 2 sources of truth are inconsistency. This could be a QA consideration.

Although, the biggest problem on an inconsistency between these 2 values is that the treasury contribution will go to an address not currently used by the LivepeerGovernor to execute its proposals. This can cause a small headache, but can be fixed by admins with no funds lost:

• If the Controller holds the actually desired treasury:
  • Regular flow for updating treasury address, by making a proposal to:
    • Transfer all funds from the current treasury to the new one
    • updateTimelock
  • If necessary to keep treasury contributions during the proposal, the admin can setContractInfo to the old treasury while the voting is happening, and then call setContractInfo to the new address after it is done
• If the LivepeerGovernor holds the actually desired treasury:
  • Admin calls setContractInfo immediately
  • Make a proposal to send the funds from the old treasury to the new one using the relay() function (cause only LivepeerGovernor will have the right roles to execute stuff)

[1]

and incorrect return the state of proposal heavily break external integration (user may think a proposal pass but it is not)

This cannot happen as the duplicate source of truth is only used to send more LPT to the treasury. It will not affect the LivepeerGovernor functioning.

So IMO this is mostly an admin error with no permanent loss to the protocol if it happens, which is consistent with a QA issue. I'll change our current stance here from disputed but not sure if we'll make any changes about it.

victorges (sponsor) acknowledged

@HickupHH3 Changed this back to acknowledged but I can't reopen the issue I think, maybe you can. As explained above, I think it's mostly an "admin error" (QA) as updating the treasury address has to be done in 2 places but is not catastrophic if a mistake is made.

Agree with sponsor! Thanks for the reply!

Acknowledged as QA(L)

#178: QA(R)

1L 1R

HickupHH3 marked the issue as grade-b