Skip to content

Commit

Permalink
[delta-audit] bonding: Various fixes from audit findings (#622)
Browse files Browse the repository at this point in the history 
* Add remappings file for vs code

* bonding: Avoid double-checkpoint on self-bond/unbond

* bonding: Fix solidity version for IBondingVotes

* bonding: Add lastClaimRound to delegator change event

* bonding,treasury: Fix a few documentations

QA Report code-423n4/2023-08-livepeer-findings#150

* bonding: Improve docs about unused _endRound

Reported on: code-423n4/2023-08-livepeer-findings#205

* bonding: Address PR comments

* bonding: Add deprecation notice to slashTranscoder

We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
  • Loading branch information
@victorges
victorges committed Sep 27, 2023
1 parent 77f8a97 commit a28713e
Show file tree
Hide file tree
Showing 7 changed files with 201 additions and 39 deletions.
56 changes: 32 additions & 24 deletions contracts/bonding/BondingManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
uint256 feeShare; // % of fees paid to delegators by transcoder
mapping(uint256 => EarningsPool.Data) earningsPoolPerRound; // Mapping of round => earnings pool for the round
uint256 lastActiveStakeUpdateRound; // Round for which the stake was last updated while the transcoder is active
uint256 activationRound; // Round in which the transcoder became active - 0 if inactive
uint256 activationRound; // Round in which the transcoder became active - if inactive will be 0 or <=deactivationRound
uint256 deactivationRound; // Round in which the transcoder will become inactive
uint256 activeCumulativeRewards; // The transcoder's cumulative rewards that are active in the current round
uint256 cumulativeRewards; // The transcoder's cumulative rewards (earned via the its active staked rewards and its reward cut).
Expand Down Expand Up @@ -135,7 +135,6 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
* should be used to initialize state variables post-deployment:
* - setUnbondingPeriod()
* - setNumActiveTranscoders()
* - setMaxEarningsClaimsRounds()
* @param _controller Address of Controller that this contract will be registered with
*/
constructor(address _controller) Manager(_controller) {}
Expand Down Expand Up @@ -352,7 +351,9 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
}

/**
* @notice Slash a transcoder. Only callable by the Verifier
* @notice Slash a transcoder. Only callable by the Verifier.
* @dev DEPRECATED: This function is not currently used in the protocol and the Verifier role is not configured. Its
* implementation is not compatible with the rest of the BondingManager code anymore.
* @param _transcoder Transcoder address
* @param _finder Finder that proved a transcoder violated a slashing condition. Null address if there is no finder
* @param _slashAmount Percentage of transcoder bond to be slashed
Expand Down Expand Up @@ -409,7 +410,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {

/**
* @notice Claim token pools shares for a delegator from its lastClaimRound through the end round
* @param _endRound The last round for which to claim token pools shares for a delegator
* @param _endRound Unused, but used to represented the last round for which to claim token pools shares for a
* delegator. Currently, the earnings are always claimed until the current round instead.
*/
function claimEarnings(uint256 _endRound)
external
Expand Down Expand Up @@ -551,6 +553,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
delegationAmount = delegationAmount.add(currentBondedAmount);

decreaseTotalStake(currentDelegate, currentBondedAmount, _oldDelegateNewPosPrev, _oldDelegateNewPosNext);
// no need to prevent double checkpointing since _owner is not a transcoder (i.e. currentDelegate != _owner)
_checkpointBondingState(currentDelegate, delegators[currentDelegate], transcoders[currentDelegate]);
}

{
Expand All @@ -574,6 +578,10 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
del.bondedAmount = currentBondedAmount.add(_amount);

increaseTotalStake(_to, delegationAmount, _currDelegateNewPosPrev, _currDelegateNewPosNext);
if (_to != _owner) {
// Avoid double checkpointing of the transcoder if it's a self-bond
_checkpointBondingState(_to, delegators[_to], transcoders[_to]);
}

if (_amount > 0) {
// Transfer the LPT to the Minter
Expand Down Expand Up @@ -745,6 +753,10 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {

// If msg.sender was resigned this statement will only decrease delegators[currentDelegate].delegatedAmount
decreaseTotalStake(currentDelegate, _amount, _newPosPrev, _newPosNext);
if (currentDelegate != msg.sender) {
// Avoid double checkpointing of the transcoder if it's a self-unbond
_checkpointBondingState(currentDelegate, delegators[currentDelegate], transcoders[currentDelegate]);
}

emit Unbond(currentDelegate, msg.sender, unbondingLockId, _amount, withdrawRound);
}
Expand Down Expand Up @@ -849,7 +861,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
/**
* @notice Returns pending bonded stake for a delegator from its lastClaimRound through an end round
* @param _delegator Address of delegator
* @param _endRound The last round to compute pending stake from
* @param _endRound Unused, but used to represent the last round to compute pending stake from. Currently, the
* pending stake is always calculated for the current round instead.
* @return Pending bonded stake for '_delegator' since last claiming rewards
*/
function pendingStake(address _delegator, uint256 _endRound) public view returns (uint256) {
Expand All @@ -864,7 +877,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
/**
* @notice Returns pending fees for a delegator from its lastClaimRound through an end round
* @param _delegator Address of delegator
* @param _endRound The last round to compute pending fees from
* @param _endRound Unused, but used to represent the last round to compute pending fees from. Currently, the
* pending fees are always calculated for the current round instead.
* @return Pending fees for '_delegator' since last claiming fees
*/
function pendingFees(address _delegator, uint256 _endRound) public view returns (uint256) {
Expand Down Expand Up @@ -1222,7 +1236,8 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
}

/**
* @dev Increase the total stake for a delegate and updates its 'lastActiveStakeUpdateRound'
* @dev Increase the total stake for a delegate and updates its 'lastActiveStakeUpdateRound'. Notice that this
* function does not checkpoint the delegate and callers should take care of it themselves.
* @param _delegate The delegate to increase the stake for
* @param _amount The amount to increase the stake for '_delegate' by
*/
Expand All @@ -1231,19 +1246,6 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
uint256 _amount,
address _newPosPrev,
address _newPosNext
) internal autoCheckpoint(_delegate) {
return increaseTotalStakeUncheckpointed(_delegate, _amount, _newPosPrev, _newPosNext);
}

/**
* @dev Implementation of increaseTotalStake that does not checkpoint the caller, to be used by functions that
* guarantee the checkpointing themselves.
*/
function increaseTotalStakeUncheckpointed(
address _delegate,
uint256 _amount,
address _newPosPrev,
address _newPosNext
) internal {
Transcoder storage t = transcoders[_delegate];

Expand Down Expand Up @@ -1289,7 +1291,7 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
uint256 _amount,
address _newPosPrev,
address _newPosNext
) internal autoCheckpoint(_delegate) {
) internal {
Transcoder storage t = transcoders[_delegate];

uint256 currStake = transcoderTotalStake(_delegate);
Expand Down Expand Up @@ -1420,7 +1422,7 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
// the earnings claiming algorithm and instead that amount is accounted for in the transcoder's cumulativeRewards field
earningsPool.updateCumulativeRewardFactor(prevEarningsPool, delegatorsRewards);
// Update transcoder's total stake with rewards
increaseTotalStakeUncheckpointed(_transcoder, _rewards, _newPosPrev, _newPosNext);
increaseTotalStake(_transcoder, _rewards, _newPosPrev, _newPosNext);
}

/**
Expand Down Expand Up @@ -1514,9 +1516,15 @@ contract BondingManager is ManagerProxyTarget, IBondingManager {
// Delete lock
delete del.unbondingLocks[_unbondingLockId];

increaseTotalStake(del.delegateAddress, amount, _newPosPrev, _newPosNext);
address delegate = del.delegateAddress;

increaseTotalStake(delegate, amount, _newPosPrev, _newPosNext);
if (delegate != _delegator) {
// Avoid double checkpointing of the transcoder if it's a self-rebond
_checkpointBondingState(delegate, delegators[delegate], transcoders[delegate]);
}

emit Rebond(del.delegateAddress, _delegator, _unbondingLockId, amount);
emit Rebond(delegate, _delegator, _unbondingLockId, amount);
}

/**
Expand Down
16 changes: 11 additions & 5 deletions contracts/bonding/BondingVotes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ contract BondingVotes is ManagerProxyTarget, IBondingVotes {
}

/**
* @dev Stores a list of checkpoints for the total active stake, queryable and mapped by round. Notce that
* @dev Stores a list of checkpoints for the total active stake, queryable and mapped by round. Notice that
* differently from bonding checkpoints, it's only accessible on the specific round. To access the checkpoint for a
* given round, look for the checkpoint in the {data}} and if it's zero ensure the round was actually checkpointed on
* the {rounds} array ({SortedArrays-findLowerBound}).
Expand Down Expand Up @@ -406,8 +406,14 @@ contract BondingVotes is ManagerProxyTarget, IBondingVotes {

// Always send delegator events since transcoders are delegators themselves. The way our rewards work, the
// delegator voting power calculated from events will only reflect their claimed stake without pending rewards.
if (previous.bondedAmount != current.bondedAmount) {
emit DelegatorBondedAmountChanged(_account, previous.bondedAmount, current.bondedAmount);
if (previous.bondedAmount != current.bondedAmount || previous.lastClaimRound != current.lastClaimRound) {
emit DelegatorBondedAmountChanged(
_account,
previous.bondedAmount,
previous.lastClaimRound,
current.bondedAmount,
current.lastClaimRound
);
}
}

Expand Down Expand Up @@ -472,8 +478,8 @@ contract BondingVotes is ManagerProxyTarget, IBondingVotes {
);

if (rewardRound < bond.lastClaimRound) {
// If the transcoder hasn't called reward() since the last time the delegator claimed earnings, there wil be
// no rewards to add to the delegator's stake so we just return the originally bonded amount.
// If the transcoder hasn't called reward() since the last time the delegator claimed earnings, there will
// be no rewards to add to the delegator's stake so we just return the originally bonded amount.
return bond.bondedAmount;
}

Expand Down
10 changes: 8 additions & 2 deletions contracts/bonding/IBondingVotes.sol
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.9;
pragma solidity 0.8.9;

import "../treasury/IVotes.sol";

Expand All @@ -24,7 +24,13 @@ interface IBondingVotes is IVotes {
* from IERC5805 by also supporting voting power for the delegators themselves, though requiring knowledge about our
* specific reward-claiming protocol to calculate voting power based on this value.
*/
event DelegatorBondedAmountChanged(address indexed delegate, uint256 previousBondedAmount, uint256 newBondedAmount);
event DelegatorBondedAmountChanged(
address indexed delegate,
uint256 previousBondedAmount,
uint256 previousLastClaimRound,
uint256 newBondedAmount,
uint256 newLastClaimRound
);

// BondingManager hooks

Expand Down
2 changes: 1 addition & 1 deletion contracts/treasury/GovernorCountingOverridable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ abstract contract GovernorCountingOverridable is Initializable, GovernorUpgradea
mapping(uint256 => ProposalTally) private _proposalTallies;

/**
* @notice The required percentage of "for" votes in relation to the total opinionated votes (for and abstain) for
* @notice The required percentage of "for" votes in relation to the total opinionated votes (for and against) for
* a proposal to succeed. Represented as a MathUtils percentage value (e.g. 6 decimal places).
*/
uint256 public quota;
Expand Down
8 changes: 8 additions & 0 deletions remappings.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@ensdomains/=node_modules/@ensdomains/
@openzeppelin/=node_modules/@openzeppelin/
contracts/=contracts/
ds-test/=lib/ds-test/src/
eth-gas-reporter/=node_modules/eth-gas-reporter/
hardhat-deploy/=node_modules/hardhat-deploy/
hardhat/=node_modules/hardhat/
sol-explore/=node_modules/sol-explore/
100 changes: 98 additions & 2 deletions test/unit/BondingManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1853,7 +1853,7 @@ describe("BondingManager", () => {
})

it("should checkpoint the delegator and transcoder states", async () => {
// make sure trancoder has a non-null `lastRewardRound`
// make sure transcoder has a non-null `lastRewardRound`
await bondingManager.connect(transcoder0).reward()

const tx = await bondingManager
Expand All @@ -1870,7 +1870,7 @@ describe("BondingManager", () => {
delegateAddress: transcoder0.address,
delegatedAmount: 2000,
lastClaimRound: currentRound - 1,
lastRewardRound: 100
lastRewardRound: currentRound
},
{
account: delegator.address,
Expand All @@ -1883,6 +1883,25 @@ describe("BondingManager", () => {
}
)
})

it("should checkpoint transcoder only once on self-bond", async () => {
// make sure transcoder has a non-null `lastRewardRound`
await bondingManager.connect(transcoder0).reward()

const tx = await bondingManager
.connect(transcoder0)
.bond(1000, transcoder0.address)

await expectCheckpoints(fixture, tx, {
account: transcoder0.address,
startRound: currentRound + 1,
bondedAmount: 2000,
delegateAddress: transcoder0.address,
delegatedAmount: 2000,
lastClaimRound: currentRound,
lastRewardRound: currentRound
})
})
})

describe("bondForWithHint", () => {
Expand Down Expand Up @@ -2577,6 +2596,22 @@ describe("BondingManager", () => {
)
})

it("should checkpoint transcoder only once on self-unbond", async () => {
// make sure transcoder has a non-null `lastRewardRound`
await bondingManager.connect(transcoder).reward()
const tx = await bondingManager.connect(transcoder).unbond(500)

await expectCheckpoints(fixture, tx, {
account: transcoder.address,
startRound: currentRound + 2,
bondedAmount: 500,
delegateAddress: transcoder.address,
delegatedAmount: 1500,
lastClaimRound: currentRound + 1,
lastRewardRound: currentRound + 1
})
})

describe("partial unbonding", () => {
it("should create an unbonding lock for a partial unbond", async () => {
const unbondingLockID = (
Expand Down Expand Up @@ -3219,6 +3254,28 @@ describe("BondingManager", () => {
)
})

it("should checkpoint transcoder only once on self-rebond", async () => {
// make sure transcoder has a non-null `lastRewardRound`
await bondingManager.connect(transcoder).reward()

// unbonding lock id will be the same
await bondingManager.connect(transcoder).unbond(500)

const tx = await bondingManager
.connect(transcoder)
.rebond(unbondingLockID)

await expectCheckpoints(fixture, tx, {
account: transcoder.address,
startRound: currentRound + 2,
bondedAmount: 1000,
delegateAddress: transcoder.address,
delegatedAmount: 1500,
lastClaimRound: currentRound + 1,
lastRewardRound: currentRound + 1
})
})

describe("current delegate is a registered transcoder", () => {
it("should increase transcoder's delegated stake in pool", async () => {
await bondingManager.connect(delegator).rebond(unbondingLockID)
Expand Down Expand Up @@ -4078,6 +4135,45 @@ describe("BondingManager", () => {
}
)
})

it("should checkpoint transcoders only once", async () => {
// make sure trancoder has a non-null `lastRewardRound`
await bondingManager.connect(transcoder0).reward()

const tx = await bondingManager
.connect(transcoder1)
.transferBond(
transcoder0.address,
999,
ZERO_ADDRESS,
ZERO_ADDRESS,
ZERO_ADDRESS,
ZERO_ADDRESS
)

await expectCheckpoints(
fixture,
tx,
{
account: transcoder1.address,
startRound: currentRound + 4,
bondedAmount: 1001,
delegateAddress: transcoder1.address,
delegatedAmount: 3001,
lastClaimRound: currentRound + 3,
lastRewardRound: 0
},
{
account: transcoder0.address,
startRound: currentRound + 4,
bondedAmount: 1999,
delegateAddress: transcoder0.address,
delegatedAmount: 3999,
lastClaimRound: currentRound + 3,
lastRewardRound: currentRound + 3
}
)
})
})

describe("receiver is bonded to zero address", () => {
Expand Down
Loading

0 comments on commit a28713e

Please sign in to comment.