Livepeer Onchain Treasury Upgrade - Vagner's results

Lines of code

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

Vulnerability details

Impact

Slashed transcoders can still become active transcorders by bonding an amount again to increase the total stake, which can inflate the actual delegatedAmount, giving those transcorders more power voting power than it should.

Proof of Concept

Every time someone bonds or unbonds tokens in the system, it will modify different storage values, by passing trough two important functions, increaseTotalStake and decreaseTotalStake. Each of these functions modify the delegatedAmount by adding or subtracting the specific amount a delegator or a transcoder bounded or unbounded from the protocol https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1344 https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1383 Delegated amount is an important variable that can be used later, after the checkpoints in BondingVotes.sol trough _checkpointBondingState function, by transcoders to weight their voting power in LivepeerGovernor.sol. The problem relies in the case of a slashed transcoder, since the delegated amount that he has is not updated in every case, which can mess up calculations in the long run. As you can see slashTranscoder can be called to slash a specific bonded amount of a transcoder and resign him from the transcoderPool https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L394-L408 amount which is calculated as a percentage of the total amount bonded and then subtracted from his total amount bonded https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L403 https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L411 In the next if statement , if there is still a bonded amount, then it will subtract the penalty also from his delegatedAmount https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L414-L417 and then burns/transfers the penalty and fees to the finder, if that is the case. The problem relies in the case where a transcoder gets all of his bonded amount slashed, because in that case the if statement here https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L414 will not pass since the delegator status will be unbounded https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L959-L961 which will not make any subtraction from the total delegatedAmount, leaving the slashed amount of the transcoder still in the delegatedAmount since it doesn't get subtracted anywhere else. Now even if the transcoder gets resign here https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L407 he can still bond any amount which will increaseTotalStake and will check if isRegisteredTranscoder https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1318 which will be the case here since isRegisteredTranscoder checks for these 2 conditions https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1158 which will both be true, since the delegateAddress is not getting deleted anywhere in the slashTranscoder or any other modifier, and bondedAmount will be greater than 0 since the bonding happens before increasing the total stake amount. Because of that the slashed transcoder will tryToJoinActiveSet and he can become a transcoder again. When the delegatedAmount gets updated after https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L1344 all of his slashed amount will still be existing into the variable, even though the amount got burn, and the new amount will be added, which will inflate his actual delegatedAmount, since his first share should not be there anymore because it got burn. That will checkpoint the transcoder with and inflated amount, which will give him more voting power in LivepeerGovernor.sol than he should have.

Tools Used

Manual review

Recommended Mitigation Steps

Consider subtracting the amount burned from the delegatedAmount all the time, since any amount he bond/undbond will increase/decrease the delegatedAmount every time beside this special case where the actual value of delegatedAmount can get inflated.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a transcoder gets slashed fully he can still vote with 0 amount of weight making any other delegated user that wants to change his vote to subtract their weight amount from other delegators/transcoders.

Proof of Concept

In BondingManager.sol any transcoder can gets slashed by a specific percentage, and that specific transcoder gets resigned and that specific percentage gets deducted from his bondedAmount https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L394-L411 If any bondedAmount will remain then the penalty will also gets subtracted from the delegatedAmount, if not, nothing happens https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L412-L417 After that the penalty gets burned and the fees gets paid to the finder, if that is the case https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L420-L440 The problem relies in the fact that a fully slashed transcoder, even if it gets resigned, he is still seen as an active transcoder in the case of voting. Let's assume this case :

• a transcoder gets fully slashed and gets resigned from the transcoder pools, getting his bondedAmount to 0, but he still has delegatedAmount to his address since nothing happens this variable https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingManager.sol#L402-L418
• transcoder vote a proposal and when his weighting gets calculated here https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/f34a3a7e5a1d698d87d517fda698d48286310bee/contracts/governance/GovernorUpgradeable.sol#L581, it will use the _getVotes from GovernorVotesUpgradeable.sol https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/f34a3a7e5a1d698d87d517fda698d48286310bee/contracts/governance/extensions/GovernorVotesUpgradeable.sol#L55-L61
• _getVotes calls getPastVotes on BondingVotes.sol https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingVotes.sol#L167-L170 which returns the amount of weight specific to this transcoder and as you can see, because the transcoder has a bondedAmount equal to 0, the first if statement will be true and 0 will be returned https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingVotes.sol#L372-L373
• 0 weight will be passed into _countVote which will then be passed into _handleVoteOverrides https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/treasury/GovernorCountingOverridable.sol#L151
• then it will check if the caller is a transcoder, which will be true in our case, because nowhere in the slashTranscoder function, or any other function the transcoder delegateAddress gets changed, so this if statement will be true https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/treasury/GovernorCountingOverridable.sol#L182-L184, which will try to deduct the weight from any previous delegators https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/treasury/GovernorCountingOverridable.sol#L184-L189
• if any delegator already overridden any amount this subtraction would revert, but if that is not the case, 0 weight will be returned, which is then used to vote for, against , abstain, but since 0 weight is passed no changed will be made.
• now the big problem arise, if any delegator that delegated their votes to this specific transcoder want to change their vote, when his weight gets calculated, delegatorCumulativeStakeAt gets called https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/bonding/BondingVotes.sol#L459-L487 which will return most of the time his bondedAmount, amount which is greater than 0, since he didn't unbound.
• because of that when _handleVoteOverrides gets called in _countVote, to override the vote, this if statement will be true, since the transcoder voted already, but with 0 weight https://github.com/code-423n4/2023-08-livepeer/blob/a3d801fa4690119b6f96aeb5508e58d752bda5bc/contracts/treasury/GovernorCountingOverridable.sol#L195 and the delegator weight gets subtracted from the support that the transcoder used in his vote
• the system in this case expect the transcoder to vote with the whole delegatedAmount, which will make the subtraction viable, since the weight of the delegator should be included in the full delegatedAmount of that specific transcoder, but since the transcoder voted with 0 weight, the subtraction would be done from other delegators/transcoders votes.
• also this can be abused by a transcoder by voting a category which he knows will not get a lot of votes, if let's say a transcoder used his 0 weight to vote for abstain and every other voter will vote on for or against, every time one of his delegators want to change the vote the subtraction can revert, which will force those delegators out of the vote, until they will change their transcoder

Tools Used

Manual review

Recommended Mitigation Steps

If a transcoder gets fully slashed and resigned, delete his address from delegateAddress so he will not appear as a transcoder in the mechanism of counting the votes. If he still wants to participate in the system he can act as a delegator to another transcoder. Another solution would be to not let 0 weight votes happen anyways, since they don't modify the vote state at all.

Assessed type

Governance