Treasury address cannot be updated in Govnor contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

In Governor contract,

we have a function

    function bumpGovernorVotesTokenAddress() external {
            token = votes();
    }

when the token address is updated, the token can be sycned

However, if the controller owner update the treasury address by calling in controller code

    /**
     * @notice Register contract id and mapped address
     * @param _id Contract id (keccak256 hash of contract name)
     * @param _contractAddress Contract address
     */
    function setContractInfo(
        bytes32 _id,
        address _contractAddress,
        bytes20 _gitCommitHash
    ) external override onlyOwner {
        registry[_id].contractAddress = _contractAddress;
        registry[_id].gitCommitHash = _gitCommitHash;

        emit SetContractInfo(_id, _contractAddress, _gitCommitHash);
    }

the governor contract treasury address and controller treasury address is out of sync

Tools Used

Manual Review

Recommended Mitigation Steps

add a function to bump treasury address as well

Assessed type

Governance

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[victorges]

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

[c4-sponsor]

victorges (sponsor) acknowledged

[c4-judge]

HickupHH3 marked the issue as selected for report

[c4-judge]

HickupHH3 marked the issue as satisfactory

[victorges]

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

[HickupHH3]

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.

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[HickupHH3]

1L

[c4-sponsor]

victorges (sponsor) disputed

[victorges]

@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.

[c4-judge]

HickupHH3 marked the issue as grade-c

[JeffCX]

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

[victorges]

@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.

[c4-sponsor]

victorges (sponsor) acknowledged

[victorges]

@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.

[JeffCX]

Agree with sponsor! Thanks for the reply!

[HickupHH3]

Acknowledged as QA(L)

[HickupHH3]

178: QA(R)

1L 1R

[c4-judge]

HickupHH3 marked the issue as grade-b