Livepeer contest - sirhashalot's results

Handle

sirhashalot

Vulnerability details

Impact

Strings are broken into 32 byte chunks for operations. Revert error strings over 32 bytes therefore consume extra gas than shorter strings, as documented publicly.

Proof of Concept

There are dozens of examples of this gas optimization opportunity in the project, but some examples of this issue include:

• L2/gateway/L2Migrator.sol:184
• L2/gateway/L2Migrator.sol:201
• L2/gateway/L2Migrator.sol:221
• L2/gateway/L2Migrator.sol:257
• bonding/BondingManager.sol:553
• bonding/BondingManager.sol:463
• bonding/BondingManager.sol:651
• token/Minter.sol:78
• token/Minter.sol:80
• token/Minter.sol:93

Recommended Mitigation Steps

Reducing revert error strings to under 32 bytes decreases deployment time gas and runtime gas when the revert condition is met. Alternatively, use custom errors, introduced in Solidity 0.8.4: https://blog.soliditylang.org/2021/04/21/custom-errors/

Handle

sirhashalot

Vulnerability details

Impact

The ControlledGateway.sol contract specifies a custom "GOVERNOR_ROLE" value that is assigned to the _msgsender when the contract is deployed. There is no need to create a custom role when only one role is used in the contract. This custom "GOVERNOR_ROLE" could be replaced with the built-in "DEFAULT_ADMIN_ROLE" value, which is the approach in the contract L1/escrow/L1Escrow.sol.

Proof of Concept

The custom role is created on line 13 of ControlledGateway.sol

Recommended Mitigation Steps

Remove the GOVERNOR_ROLE role in ControlledGateway.sol and use the built-in DEFAULT_ADMIN_ROLE role to save gas

Handle

sirhashalot

Vulnerability details

Impact

The currentBondingRate variable in the Minter.sol contract of the livepeer/protocol project is initialized to a value of 0. Since currentBondingRate is a uint256, the default value is zero per Solidity docs: https://docs.soliditylang.org/en/latest/control-structures.html#default-value

Setting a variable to the default value it already contains is unnecessary. Removing this line would save gas.

Proof of Concept

uint256 currentBondingRate = 0;

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/token/Minter.sol#L223

Recommended Mitigation Steps

Modify the initialization of currentBondingRate to:

uint256 currentBondingRate;

Handle

sirhashalot

Vulnerability details

Impact

Polls require the proposer to burn tokens, so they are a serious matter. However, the vote() function in Poll.sol of the primary Livepeer project has several flaws:

1. Anyone with a valid address can vote. It is unlikely that the intent for these polls is for everyone in the Ethereum ecosystem to vote
2. There is no limit around duplicate voting. The comments indicate the Vote event "can be indexed to tally all votes for each choiceID", which implies no checks on the address of the vote event. Even if a manual check which is not documented does occur, performing such an important check off-chain can be flawed in a variety of ways.

These flaws allow an attacker to have unlimited votes, deciding the winning result of the poll. Even if malicious activity is detected, the tokens for the poll have already been burned and are irretrievable.

The scope of this contest was defined below. Since this poll voting issue is from "those contracts", I considered it in scope.

The recommendation for wardens is to focus on the LIP-73 contracts - links to the protocol contracts are provided as well for background/reference, however, if there are any findings surfaced in those contracts during the contest those are certainly welcome as well.

Proof of Concept

The relevant code is:

modifier isActive() {
    require(block.number <= endBlock, "poll is over");
    _;
}

/**
 * @dev Vote for the poll's proposal.
 *      Reverts if the poll period is over.
 * @param _choiceID the ID of the option to vote for
 */
function vote(uint256 _choiceID) external isActive {
    emit Vote(msg.sender, _choiceID);
}

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/polling/Poll.sol#L25

Recommended Mitigation Steps

This vote mechanism is very simple and lacks proper checks. Consider using a more robust and tested voting mechanism, such as OpenZeppelin's Governance contract or Compound's GovernorAlpha or GovernorBravo contracts. In order to make it easier to undo a poll that was attacked, consider depositing the tokens for the poll creation into a governance multisig wallet instead of burning them, to allow for easier recovery from poll manipulation.