[delta-audit] bonding: Provide votes only for registered Ts and their Ds

[victorges]

What does this pull request do? Explain your changes. (required)
This fixes another issue from the audit regarding voting power provided to delegators. The specific issue is when a delegator has a bond with an account that is not a "registered transcoder" (delegateAddress == self && bondedAmount > 0).

This can happen when bonding to a random account that is not a transcoder, when the transcoder unbonds or gets slashed 100%. The main problem created from this is that the transcoder would then have 0 voting power, while the delegators would still hold on to their delegated voting power, messing up vote overriding on GovenorCountingOverridable. Even with the #626 fix, there would still be a problem in case bondedAmount == 0 but still delegateAddress == self (can happen in the slash case).

Specific updates (required)

• Update BondingVotes to not give voting power to delegators to non-transcoders (transcoder defined as self-delegate a non 0 amount)
• Add tests

How did you test each of these updates (required)
yarn test with pending devnet deploy

Does this pull request close any open issues?
Fixes:

• Fully slashed transcoder can vote with 0 weight messing up the voting calculations code-423n4/2023-08-livepeer-findings#194
• By delegating to a non-transcoder, a delegator can reduce the tally of somebody else's vote choice without first granting them any voting power code-423n4/2023-08-livepeer-findings#96

Checklist:

• README and other documentation updated
• All tests using yarn test pass

[yondonfu]

Not sure if this is already addressed elsewhere, but making a note that there is also a conditional in _handleVoteOverrides() of GovernorCountingOverridable that does not implement the complete registered transcoder check. This doesn't seem to cause any issues in that function at the moment given the changes in this PR, but could be a good idea to add some clarifications there. A few options come to mind:

a. Update IVotes.delegatedAt() to return (address, bool) where the bool value indicates whether the delegate is a registered transcoder
b. Update the comments around the referenced conditional explaining why checking if the address == delegate is sufficient + safe despite there not being a complete registered transcoder check since _weight = 0 and _voter.deductions = 0 if the address is not a registered transcoder

EDIT: I think #626 kind of addresses this, but a bit more info with b) might still be helpful for a future reader.

[victorges]

@yondonfu Regarding not having a full check on _handleVoteOverrides(), honestly I decided to skip it to avoid another call to votes(delegate) when casting a vote, which could significantly increase the gas costs for a delegator to vote. I think the a idea would solve this gas cost issue, tho I think it diverges from IERC5805.delegates so would feel a bit weird (could be a better-named getBondingState() instead then we make a single call?)

My rationale in the new version of the code is:

• GovernorCountingOverridable expects a single invariant: self-delegating accounts should have a voting power >= than the sum of all its delegators (only requirement for overriding logic to work properly)
  • This is why I also removed any "isTranscoder" terms from there. It only cares about self-delegation now
• BondingVotes implements that invariant, since when the delegate is self-delegate but has no bondedAmount, everyone gets 0 voting power

So it'd be an "implementation detail" for the IVotes implementation provided. I agree it's very worth to document it on GovernorCountingOverridable tho as it could be confusing. I'll do just that but not change any of the implementation, if you think that's ok. If you have a strong preference for a or some other solution lmk tho!

[yondonfu]

I agree it's very worth to document it on GovernorCountingOverridable tho as it could be confusing. I'll do just that but not change any of the implementation, if you think that's ok

Sounds reasonable.

[victorges]

Just realized tests are failing after the merge. Brb.

[victorges]

Fixed!

[victorges]

@yondonfu Fixed