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

… Ds (#625)

* bonding: Make consistent checks for isTranscoder

* bonding: Add tests for isRegisteredTranscoder logic

* treasury: Improve docs in vote overriding extension

* bonding: Rename getBondingStateAt for accuracy

* bonding: Fix BondingVotes test

* bonding: Fix retval ordering in docs

contracts/bonding/IBondingVotes.sol

@@ -52,7 +52,7 @@ interface IBondingVotes is IVotes {
 
     function getTotalActiveStakeAt(uint256 _round) external view returns (uint256);
 
-    function getBondingStateAt(address _account, uint256 _round)
+    function getVotesAndDelegateAtRoundStart(address _account, uint256 _round)
         external
         view
         returns (uint256 amount, address delegateAddress);

contracts/test/mocks/BondingVotesERC5805Harness.sol

@@ -16,7 +16,7 @@ contract BondingVotesERC5805Harness is BondingVotes {
      * @return amount lowest 4 bytes of address + _round
      * @return delegateAddress (_account << 4) | _round.
      */
-    function getBondingStateAt(address _account, uint256 _round)
+    function getVotesAndDelegateAtRoundStart(address _account, uint256 _round)
         public
         pure
         override

contracts/treasury/GovernorCountingOverridable.sol

@@ -16,7 +16,8 @@ import "./IVotes.sol";
 /**
  * @title GovernorCountingOverridable
  * @notice Implements the Counting module from OpenZeppelin Governor with support for delegators overriding their
- * delegated transcoder's vote. This module is used through inheritance by the Governor contract.
+ * delegated transcoder's vote. This module is used through inheritance by the Governor contract, which must implement
+ * the `votes()` function to provide an instance of the IVotes interface.
  */
 abstract contract GovernorCountingOverridable is Initializable, GovernorUpgradeable {
     error InvalidVoteType(uint8 voteType);
@@ -118,7 +119,7 @@ abstract contract GovernorCountingOverridable is Initializable, GovernorUpgradea
     function _voteSucceeded(uint256 _proposalId) internal view virtual override returns (bool) {
         (uint256 againstVotes, uint256 forVotes, ) = proposalVotes(_proposalId);
 
-        // we ignore abstain votes for vote succeeded calculation
+        // We ignore abstain votes for vote succeeded calculation
         uint256 opinionatedVotes = againstVotes + forVotes;
 
         return forVotes >= MathUtils.percOf(opinionatedVotes, quota);
@@ -181,25 +182,30 @@ abstract contract GovernorCountingOverridable is Initializable, GovernorUpgradea
         uint256 timepoint = proposalSnapshot(_proposalId);
         address delegate = votes().delegatedAt(_account, timepoint);
 
+        // Notice that we don't need to check if the voting power is greater than zero to trigger the override logic
+        // here, which would be the equivalent of the `bondedAmount > 0` check in {BondingVotes-isRegisteredTranscoder}.
+        // This is because if the account has zero `bondedAmount`, then it and all its delegators will also have zero
+        // voting power, meaning the `votes()` invariant still holds (`0 >= sum(N*0)`).
         bool isSelfDelegated = _account == delegate;
         if (isSelfDelegated) {
-            // deduce weight from any previous delegators for this self-delegating account to cast a vote
+            // Deduce weight from any previous delegators for this self-delegating account to cast a vote.
             return _weight - _voter.deductions;
         }
 
+        // Same as above, all we need to check here is for a self-delegation due to the `votes()` invariant.
         bool isDelegateSelfDelegated = delegate == votes().delegatedAt(delegate, timepoint);
         if (!isDelegateSelfDelegated) {
-            // do not override votes of non-self-delegating accounts since those don't get their voting power from the
+            // Do not override votes of non-self-delegating accounts since those don't get their voting power from the
             // sum of delegations to them, so the override logic doesn't apply.
             return _weight;
         }
 
-        // this is a delegator, so add a deduction to the delegated transcoder
+        // This is a delegator, so add a deduction to the delegated transcoder
         ProposalVoterState storage delegateVoter = _tally.voters[delegate];
         delegateVoter.deductions += _weight;
 
         if (delegateVoter.hasVoted) {
-            // this is a delegator overriding its delegated transcoder vote,
+            // This is a delegator overriding its delegated transcoder vote,
             // we need to update the current totals to move the weight of
             // the delegator vote to the right outcome.
             VoteType delegateSupport = delegateVoter.support;
@@ -218,11 +224,10 @@ abstract contract GovernorCountingOverridable is Initializable, GovernorUpgradea
     }
 
     /**
-     * @dev Implement in inheriting contract to provide the voting power provider.
-     *
-     * The most important expectation for the voting power provided by this contract is that, if an account delegates to
-     * itself (self-delegation) its voting power MUST be greater than or equal to the sum of all voting power from other
-     * accounts that delegate to it. If this invariant is broken, the vote overriding logic will break.
+     * @notice Provides the IVotes contract for all voting power handling logics.
+     * @dev The returned contract must guarantee this invariant at any given timepoint: if an `_account` delegates to
+     * itself (self-delegation), then its voting power must be greater than or equal to the sum of the voting power of
+     * all its delegators. In pseudo-code: `votes(_account) >= sum(votes(delegators(_account)))`
      */
     function votes() public view virtual returns (IVotes);
 

src/test/BondingVotesStateInitialization.sol

@@ -108,7 +108,7 @@ contract BondingVotesStateInitialization is GovernorBaseTest {
         uint256 currentRound = ROUNDS_MANAGER.currentRound();
 
         for (uint256 i = 0; i < _testAddresses.length; i++) {
-            (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getBondingStateAt(
+            (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getVotesAndDelegateAtRoundStart(
                 _testAddresses[i],
                 currentRound
             );
@@ -128,11 +128,11 @@ contract BondingVotesStateInitialization is GovernorBaseTest {
 
             // Still returns zero checkpoint in the current round, checkpoint is made for the next.
             // We don't check delegatedAmount for simplicity here, it is checked in the other tests.
-            (, address checkedDelegate) = bondingVotes.getBondingStateAt(addr, currentRound);
+            (, address checkedDelegate) = bondingVotes.getVotesAndDelegateAtRoundStart(addr, currentRound);
             assertEq(checkedDelegate, address(0));
 
             // Allows querying up to the next round.
-            (, checkedDelegate) = bondingVotes.getBondingStateAt(addr, currentRound + 1);
+            (, checkedDelegate) = bondingVotes.getVotesAndDelegateAtRoundStart(addr, currentRound + 1);
             assertEq(
                 checkedDelegate,
                 addr == DELEGATOR || addr == DELEGATOR_DELEGATE ? DELEGATOR_DELEGATE : addr == TRANSCODER
@@ -144,7 +144,7 @@ contract BondingVotesStateInitialization is GovernorBaseTest {
             CHEATS.expectRevert(
                 abi.encodeWithSelector(IBondingVotes.FutureLookup.selector, currentRound + 2, currentRound + 1)
             );
-            bondingVotes.getBondingStateAt(addr, currentRound + 2);
+            bondingVotes.getVotesAndDelegateAtRoundStart(addr, currentRound + 2);
         }
     }
 
@@ -154,7 +154,10 @@ contract BondingVotesStateInitialization is GovernorBaseTest {
 
         BONDING_MANAGER.checkpointBondingState(TRANSCODER);
 
-        (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getBondingStateAt(TRANSCODER, currentRound + 1);
+        (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getVotesAndDelegateAtRoundStart(
+            TRANSCODER,
+            currentRound + 1
+        );
         assertEq(checkedAmount, delegatedAmount);
         assertEq(checkedDelegate, TRANSCODER);
     }
@@ -170,7 +173,10 @@ contract BondingVotesStateInitialization is GovernorBaseTest {
         // the delegate also needs to be checkpointed in case of delegators
         BONDING_MANAGER.checkpointBondingState(DELEGATOR_DELEGATE);
 
-        (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getBondingStateAt(DELEGATOR, currentRound + 1);
+        (uint256 checkedAmount, address checkedDelegate) = bondingVotes.getVotesAndDelegateAtRoundStart(
+            DELEGATOR,
+            currentRound + 1
+        );
 
         assertEq(checkedAmount, pendingStake);
         assertEq(checkedDelegate, DELEGATOR_DELEGATE);

test/gas-report/checkpoints.js

@@ -111,31 +111,41 @@ describe("checkpoint bonding state gas report", () => {
         })
     })
 
-    describe("getBondingStateAt", () => {
+    describe("getVotesAndDelegateAtRoundStart", () => {
         beforeEach(async () => {
             await bondingManager.checkpointBondingState(transcoder.address)
             await bondingManager.checkpointBondingState(delegator.address)
             await bondingManager.checkpointBondingState(signers[99].address)
         })
 
-        const gasGetBondingStateAt = async (address, round) => {
-            const tx = await bondingVotes.populateTransaction.getBondingStateAt(
-                address,
-                round
-            )
+        const gasGetVotesAndDelegateAtRoundStart = async (address, round) => {
+            const tx =
+                await bondingVotes.populateTransaction.getVotesAndDelegateAtRoundStart(
+                    address,
+                    round
+                )
             await signers[0].sendTransaction(tx)
         }
 
         it("delegator", async () => {
-            await gasGetBondingStateAt(delegator.address, currentRound + 1)
+            await gasGetVotesAndDelegateAtRoundStart(
+                delegator.address,
+                currentRound + 1
+            )
         })
 
         it("transcoder", async () => {
-            await gasGetBondingStateAt(transcoder.address, currentRound + 1)
+            await gasGetVotesAndDelegateAtRoundStart(
+                transcoder.address,
+                currentRound + 1
+            )
         })
 
         it("non-participant", async () => {
-            await gasGetBondingStateAt(signers[99].address, currentRound + 1)
+            await gasGetVotesAndDelegateAtRoundStart(
+                signers[99].address,
+                currentRound + 1
+            )
         })
     })
 })

test/integration/BondingVotes.js

@@ -142,7 +142,7 @@ describe("BondingVotes", () => {
             await nextRound()
         })
 
-        describe("getBondingStateAt", () => {
+        describe("getVotesAndDelegateAtRoundStart", () => {
             it("should return partial rewards for any rounds since bonding", async () => {
                 const pendingRewards0 = math.precise.percOf(
                     mintableTokens[currentRound].div(2), // 50% cut rate
@@ -157,7 +157,10 @@ describe("BondingVotes", () => {
 
                 const stakeAt = round =>
                     bondingVotes
-                        .getBondingStateAt(delegator.address, round)
+                        .getVotesAndDelegateAtRoundStart(
+                            delegator.address,
+                            round
+                        )
                         .then(n => n[0].toString())
 
                 assert.equal(await stakeAt(2), 0)
@@ -176,7 +179,10 @@ describe("BondingVotes", () => {
             it("should return partial rewards for all transcoder stake", async () => {
                 const stakeAt = round =>
                     bondingVotes
-                        .getBondingStateAt(transcoder.address, round)
+                        .getVotesAndDelegateAtRoundStart(
+                            transcoder.address,
+                            round
+                        )
                         .then(n => n[0].toString())
 
                 assert.equal(await stakeAt(2), 0)
@@ -318,16 +324,17 @@ describe("BondingVotes", () => {
             assert.isFalse(await isActive(inactiveTranscoder.address))
         })
 
-        describe("getBondingStateAt", () => {
+        describe("getVotesAndDelegateAtRoundStart", () => {
             it("should provide voting power even for inactive transcoders and their delegators", async () => {
                 const transcoder = transcoders[transcoders.length - 1].address
                 const delegator = delegators[delegators.length - 1].address
 
                 const testHasStake = async (address, round) => {
-                    const [stake] = await bondingVotes.getBondingStateAt(
-                        address,
-                        round
-                    )
+                    const [stake] =
+                        await bondingVotes.getVotesAndDelegateAtRoundStart(
+                            address,
+                            round
+                        )
                     assert.isAbove(
                         stake,
                         0,
@@ -354,7 +361,10 @@ describe("BondingVotes", () => {
                         const expectedStake = pendingStakes[address]
 
                         const [stakeCheckpoint] =
-                            await bondingVotes.getBondingStateAt(address, round)
+                            await bondingVotes.getVotesAndDelegateAtRoundStart(
+                                address,
+                                round
+                            )
                         assert.equal(
                             stakeCheckpoint.toString(),
                             expectedStake,
@@ -382,10 +392,11 @@ describe("BondingVotes", () => {
                 for (const r = currentRound - 2; r <= currentRound + 2; r++) {
                     const activeStakeSum = BigNumber.from(0)
                     for (const transcoder of activeTranscoders) {
-                        const [stake] = await bondingVotes.getBondingStateAt(
-                            transcoder.address,
-                            r
-                        )
+                        const [stake] =
+                            await bondingVotes.getVotesAndDelegateAtRoundStart(
+                                transcoder.address,
+                                r
+                            )
                         activeStakeSum = activeStakeSum.add(stake)
                     }
 
@@ -458,10 +469,10 @@ describe("BondingVotes", () => {
             await nextRound()
         })
 
-        describe("getBondingStateAt", () => {
+        describe("getVotesAndDelegateAtRoundStart", () => {
             const stakeAt = (account, round) =>
                 bondingVotes
-                    .getBondingStateAt(account.address, round)
+                    .getVotesAndDelegateAtRoundStart(account.address, round)
                     .then(n => n[0].toString())
             const expectStakeAt = async (account, round, expected) => {
                 assert.equal(
@@ -608,10 +619,11 @@ describe("BondingVotes", () => {
         })
 
         const expectStakeAt = async (account, round, expected, delegate) => {
-            const stakeAndAddress = await bondingVotes.getBondingStateAt(
-                account.address,
-                round
-            )
+            const stakeAndAddress =
+                await bondingVotes.getVotesAndDelegateAtRoundStart(
+                    account.address,
+                    round
+                )
             assert.equal(
                 stakeAndAddress[0].toString(),
                 expected.toString(),