User who will do any action in round before reward is called will loose rewards for that round

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-livepeer/blob/main/contracts/bonding/BondingManager.sol#L1550

Vulnerability details

Impact

User who will do any action in round before reward is called will loose rewards for that round

Proof of Concept

BondingManager operates with rounds. When user performs any action, then _autoClaimEarnings is called for him in order to claim rewards from the user's lastClaimRound till currentRound.

In order to do so, each round has earning pool data information that contains cumulativeRewardFactor variable. This variable is likely to increase from past rounds to future rounds.

This is because, transcoder can call reward function for each round, which will claim stake tokens that should be distributed among delegators. This will change cumulativeRewardFactor variable for current round.

In case if user has already claimed round, then he can't claim it again as it's stored as lastClaimRound. Because of that, in case if use did any action, for example staked more funds, then _autoClaimEarnings is called for him, which will claim current round for user. Then if transcoder will call reward for current round and cumulativeRewardFactor for it will increase, then user will not be able to receive rewards for it.

I have created test to show this. This test was created inside Rewards.js file in the test/integration folder. Copy content there and run. Pls, check my comments under the test to see how to run it and how to detect error.

import {constants} from "../../utils/constants"
import math from "../helpers/math"

import chai, {expect} from "chai"
import {solidity} from "ethereum-waffle"
import {ethers} from "hardhat"
import setupIntegrationTest from "../helpers/setupIntegrationTest"

chai.use(solidity)

describe("Rewards", () => {
    let controller
    let bondingManager
    let roundsManager
    let token

    let transcoder1
    let delegator1
    let delegator2
    let delegator3

    let rewardCut
    let feeShare
    let transcoder1StartStake
    let delegator1StartStake
    let delegator2StartStake
    let delegator3StartStake

    let roundLength

    before(async () => {
        const signers = await ethers.getSigners()

        transcoder1 = signers[0]
        delegator1 = signers[2]
        delegator2 = signers[3]
        delegator3 = signers[4]

        const fixture = await setupIntegrationTest()

        controller = await ethers.getContractAt(
            "Controller",
            fixture.Controller.address
        )
        await controller.unpause()

        bondingManager = await ethers.getContractAt(
            "BondingManager",
            fixture.BondingManager.address
        )
        roundsManager = await ethers.getContractAt(
            "AdjustableRoundsManager",
            fixture.AdjustableRoundsManager.address
        )
        token = await ethers.getContractAt(
            "LivepeerToken",
            fixture.LivepeerToken.address
        )

        const transferAmount = ethers.BigNumber.from(10).mul(
            constants.TOKEN_UNIT.toString()
        )
        await token
            .connect(signers[0])
            .transfer(transcoder1.address, transferAmount)
        await token
            .connect(signers[0])
            .transfer(delegator1.address, transferAmount)
        await token
            .connect(signers[0])
            .transfer(delegator2.address, transferAmount)
        await token
            .connect(signers[0])
            .transfer(delegator3.address, transferAmount)

        roundLength = await roundsManager.roundLength()
        await roundsManager.mineBlocks(roundLength.mul(1000))
        await roundsManager.initializeRound()

        rewardCut = 50 // 50%
        feeShare = 5 // 5%
        transcoder1StartStake = 1000
        delegator1StartStake = 3000
        delegator2StartStake = 3000
        delegator3StartStake = 3000

        // Register transcoder 1
        await token
            .connect(transcoder1)
            .approve(bondingManager.address, transcoder1StartStake)
        await bondingManager
            .connect(transcoder1)
            .bond(transcoder1StartStake, transcoder1.address)
        await bondingManager
            .connect(transcoder1)
            .transcoder(
                rewardCut * constants.PERC_MULTIPLIER,
                feeShare * constants.PERC_MULTIPLIER
            )

        // Delegator 1 delegates to transcoder 1
        await token
            .connect(delegator1)
            .approve(bondingManager.address, delegator1StartStake)
        await bondingManager
            .connect(delegator1)
            .bond(delegator1StartStake, transcoder1.address)

        // Delegator 2 delegates to transcoder 1
        await token
            .connect(delegator2)
            .approve(bondingManager.address, delegator2StartStake)
        await bondingManager
            .connect(delegator2)
            .bond(delegator2StartStake, transcoder1.address)

        // Delegator 3 delegates to transcoder 1
        await token
            .connect(delegator3)
            .approve(bondingManager.address, delegator3StartStake)
        await bondingManager
            .connect(delegator3)
            .bond(delegator3StartStake, transcoder1.address)

        await roundsManager.mineBlocks(roundLength)
        await roundsManager.initializeRound()
    })

    //about this test
    //this test shows that in case if user called claimEarnings in the period before transoder called reward
    //then user will loose rewards for that period

    //i have provided a lot of logging here to show output, this logging helps to understand how stake is increasing
    //there is a variable at the top of test: userShouldLooseRewards
    //if you set it to flase, then user will not claim before reward call and you will see that he earns more for same period of time 
    it.only("loose reward", async () => {
        //change test mode to see difference
        let userShouldLooseRewards = true;

        const callRewardAndCheckStakes = async () => {
            const calcRewardShare = (
                startStake,
                startRewardFactor,
                endRewardFactor
            ) => {
                return math.precise
                    .percOf(
                        startStake,
                        endRewardFactor,
                        startRewardFactor.toString()
                    )
                    .sub(startStake)
            }
            const acceptableDelta = ethers.BigNumber.from(
                constants.TOKEN_UNIT.toString()
            ).div(1000) // .001

            const t1StartStake = (
                await bondingManager.getDelegator(transcoder1.address)
            ).bondedAmount
            const d1StartStake = (
                await bondingManager.getDelegator(delegator1.address)
            ).bondedAmount
            const d2StartStake = (
                await bondingManager.getDelegator(delegator2.address)
            ).bondedAmount
            const d3StartStake = (
                await bondingManager.getDelegator(delegator3.address)
            ).bondedAmount

            await bondingManager.connect(transcoder1).reward()

            const currentRound = await roundsManager.currentRound()

            const lastClaimRoundT1 = (
                await bondingManager.getDelegator(transcoder1.address)
            ).lastClaimRound
            let startRewardFactor = (
                await bondingManager.getTranscoderEarningsPoolForRound(
                    transcoder1.address,
                    lastClaimRoundT1
                )
            ).cumulativeRewardFactor
            startRewardFactor =
                startRewardFactor.toString() != "0" ?
                    startRewardFactor :
                    constants.PERC_DIVISOR_PRECISE
            const endRewardFactor = (
                await bondingManager.getTranscoderEarningsPoolForRound(
                    transcoder1.address,
                    currentRound
                )
            ).cumulativeRewardFactor
            const transcoderRewards = (
                await bondingManager.getTranscoder(transcoder1.address)
            ).cumulativeRewards

            const expT1RewardShare = calcRewardShare(
                t1StartStake,
                startRewardFactor,
                endRewardFactor
            ).add(transcoderRewards)
            const expD1RewardShare = calcRewardShare(
                d1StartStake,
                startRewardFactor,
                endRewardFactor
            )
            const expD2RewardShare = calcRewardShare(
                d2StartStake,
                startRewardFactor,
                endRewardFactor
            )
            const expD3RewardShare = calcRewardShare(
                d3StartStake,
                startRewardFactor,
                endRewardFactor
            )

            const t1Stake = await bondingManager.pendingStake(
                transcoder1.address,
                currentRound
            )
            const d1Stake = await bondingManager.pendingStake(
                delegator1.address,
                currentRound
            )
            const d2Stake = await bondingManager.pendingStake(
                delegator2.address,
                currentRound
            )
            const d3Stake = await bondingManager.pendingStake(
                delegator3.address,
                currentRound
            )

            const t1RewardShare = t1Stake.sub(t1StartStake.toString())
            const d1RewardShare = d1Stake.sub(d1StartStake.toString())
            const d2RewardShare = d2Stake.sub(d2StartStake.toString())
            const d3RewardShare = d3Stake.sub(d3StartStake.toString())

            expect(t1RewardShare.sub(expT1RewardShare.toString())).to.be.lte(
                acceptableDelta
            )
            expect(d1RewardShare.sub(expD1RewardShare.toString())).to.be.lte(
                acceptableDelta
            )
            expect(d2RewardShare.sub(expD2RewardShare.toString())).to.be.lte(
                acceptableDelta
            )
            expect(d3RewardShare.sub(expD3RewardShare.toString())).to.be.lte(
                acceptableDelta
            )
        }

        const claimEarningsAndCheckStakes = async () => {
            const acceptableDelta = ethers.BigNumber.from(
                constants.TOKEN_UNIT.toString()
            ).div(1000) // .001

            const currentRound = await roundsManager.currentRound()

            const t1StartStake = await bondingManager.pendingStake(
                transcoder1.address,
                currentRound
            )
            const d1StartStake = await bondingManager.pendingStake(
                delegator1.address,
                currentRound
            )
            const d2StartStake = await bondingManager.pendingStake(
                delegator2.address,
                currentRound
            )
            const d3StartStake = await bondingManager.pendingStake(
                delegator3.address,
                currentRound
            )

            await bondingManager
                .connect(transcoder1)
                .claimEarnings(currentRound)
            await bondingManager.connect(delegator1).claimEarnings(currentRound)
            await bondingManager.connect(delegator2).claimEarnings(currentRound)
            await bondingManager.connect(delegator3).claimEarnings(currentRound)

            const t1Stake = (
                await bondingManager.getDelegator(transcoder1.address)
            ).bondedAmount
            const d1Stake = (
                await bondingManager.getDelegator(delegator1.address)
            ).bondedAmount
            const d2Stake = (
                await bondingManager.getDelegator(delegator2.address)
            ).bondedAmount
            const d3Stake = (
                await bondingManager.getDelegator(delegator3.address)
            ).bondedAmount

            expect(t1Stake.sub(t1StartStake)).to.be.lte(acceptableDelta)
            expect(d1Stake.sub(d1StartStake)).to.be.lte(acceptableDelta)
            expect(d2Stake.sub(d2StartStake)).to.be.lte(acceptableDelta)
            expect(d3Stake.sub(d3StartStake)).to.be.lte(acceptableDelta)
        }

        let d1Stake = await bondingManager.pendingStake(
            delegator1.address,
            await roundsManager.currentRound()
        )
        console.log("Pending stake before first reward call. It's initial stake.")
        console.log(Number(d1Stake._hex))

        await callRewardAndCheckStakes()
        d1Stake = await bondingManager.pendingStake(
            delegator1.address,
            await roundsManager.currentRound()
        )
        console.log("Pending stake after first reward call. User received rewards.")
        console.log(Number(d1Stake._hex))
        await roundsManager.mineBlocks(roundLength)
        await roundsManager.initializeRound()

        await callRewardAndCheckStakes()
        d1Stake = await bondingManager.pendingStake(
            delegator1.address,
            await roundsManager.currentRound()
        )
        console.log("Pending stake after second reward call. User received rewards")
        console.log(Number(d1Stake._hex))

        // Check reward accounting after calling claimEarnings
        await claimEarningsAndCheckStakes()

        await roundsManager.mineBlocks(roundLength)
        await roundsManager.initializeRound()

        await callRewardAndCheckStakes()
        d1Stake = await bondingManager.pendingStake(
            delegator1.address,
            await roundsManager.currentRound()
        )
        console.log("Pending stake after third reward call. User received rewards")
        console.log(Number(d1Stake._hex))

        // Check reward accounting after calling claimEarnings
        // Order should not matter - transcoder can claim in the middle
        await claimEarningsAndCheckStakes()

        await roundsManager.mineBlocks(roundLength)
        await roundsManager.initializeRound()

        //here we claim earning before reward is called in same period or no
        if (userShouldLooseRewards) {
            //user claims before 
            //because of that he will not receive funds for next period
            console.log("User claims earnings before transcoder calls reward in the period")

            await bondingManager.connect(delegator1).claimEarnings(await roundsManager.currentRound())

            let stakeBeforeNextReward = await bondingManager.pendingStake(
                delegator1.address,
                await roundsManager.currentRound()
            )
            await callRewardAndCheckStakes()
            let stakeAfterNextReward = await bondingManager.pendingStake(
                delegator1.address,
                await roundsManager.currentRound()
            )
            console.log("Pending stake after fourth reward call. User doesn't receive rewards for this period as he already claimed. In case if you comment claiming then you will see that stake has increased.")
            console.log(Number(stakeAfterNextReward._hex))

            //same amount after reward call
            expect(stakeBeforeNextReward).eq(stakeAfterNextReward)
        } else {
            console.log("User didn't claim earnings before transcoder calls reward in the period. As result his stake will increase")

            let stakeBeforeNextReward = await bondingManager.pendingStake(
                delegator1.address,
                await roundsManager.currentRound()
            )
            await callRewardAndCheckStakes()
            stakeAfterNextReward = await bondingManager.pendingStake(
                delegator1.address,
                await roundsManager.currentRound()
            )
            console.log("Pending stake after fourth reward call. User receive rewards for this period.")
            console.log(Number(stakeAfterNextReward._hex))

            //amount increased reward call
            expect(stakeAfterNextReward).gt(stakeBeforeNextReward)
        }

        // Check reward accounting after calling claimEarnings
        // Order should not matter - transcoder can claim last
        await claimEarningsAndCheckStakes()

        //last one round to show that he earns later
        await roundsManager.mineBlocks(roundLength)
        await roundsManager.initializeRound()

        await callRewardAndCheckStakes()
        d1Stake = await bondingManager.pendingStake(
            delegator1.address,
            await roundsManager.currentRound()
        )
        console.log("Pending stake after last reward call. User received rewards")
        console.log(Number(d1Stake._hex))

        // Check reward accounting after calling claimEarnings
        // Order should not matter - transcoder can claim in the middle
        await claimEarningsAndCheckStakes()
    })
})

Tools Used

VsCode

Recommended Mitigation Steps

I don't know good solution. This will need to change rounds handling. Maybe do not claim current round for user in case if last reward round of transcoder is less than current round.

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[victorges]

As discussed with this warden on Immunefi, the situation described is accurate, however this has long been a known behavior in the protocol. Any delegator who claims their rewards in a round, prior to their Orchestrator calling reward, forfeit that rounds' rewards. Delegators have been instructed as such in terms of the side effects of choosing to take bonding actions before their delegate has called reward.

[c4-sponsor]

victorges (sponsor) disputed

[HickupHH3]

Recognised consequence of design, marking as invalid.

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[rvierdiiev]

hello @HickupHH3 i would like to point you to some facts and hope that you will upgrade this report to valid medium 1.sponsor confirms that this is correctly described situation and test also does that. 2.known issues had no word about this as known. 3.BondingManager contract was in scope.

So this is the situation. I have spent time on this and found the issue. Even that sponsor says that it's known for all participants of protocol, doesn't actually mean that it's known for us - auditors. For such cases we have known issues section that should describe all known issues. So this protocol didn't list this concern as known. That's why i have spend time and provided test that proves my point.

I believe that as lose of yields this issue should be accepted as medium severity and sponsor should mark it as acknowledged as indeed they agree that such situation can occur, but they wouldn't like to fix it.

Also, pls, consider you own comment which also talks about the need to provide known limitations in the readme.

thanks

[HickupHH3]

Even that sponsor says that it's known for all participants of protocol, doesn't actually mean that it's known for us - auditors

This isn't exactly true, as it seems that you've reported this before and got rejected, so at least you know about it.

As discussed with this warden on Immunefi, the situation described is accurate, however this has long been a known behavior in the protocol.

It feels like a dangerous precedent to award findings that were rejected before, with the assumption that the sponsor is acting in good faith here.

Delegators have been instructed as such in terms of the side effects of choosing to take bonding actions before their delegate has called reward.

While it may not be documented, it's at least been communicated to the relevant parties.

[rvierdiiev]

This isn't exactly true, as it seems that you've reported this before and got rejected, so at least you know about it.

It was done in the same time with audit contest and with the consent(advice) of sponsor. So no, i didn't know that.

While it may not be documented, it's at least been communicated to the relevant parties.

Indeed, but i didn't know about that, as well as all auditors.

[HickupHH3]

@victorges can u verify the claim made regarding the immunefi submission, and do u have written documentation for the known behaviour?

[victorges]

Hey @HickupHH3 ! The Immunefi submission was made in concurrence with the C4 audit as a strategy to get paid from 2 different sources for the same issue. @rvierdiyev did message me on Discord about it and I told him the preferable would be submitting only on C4 audit as the contract in question was in scope [1]. Since we got the Immunefi report before the C4 audit finished, the conversation had already happened and the report was dismissed over there.

Regardless, we dismissed the report in Immunefi as it refers to a known behavior of the cumulative reward claiming logic, not due to the duplicate report. Here are some places where we've talked about this publicly:

• https://forum.livepeer.org/t/difference-between-reward-and-claimearnings/244

  CAUTION: After you call claimEarnings() for a round, you can not call it again for that round. That means that if the transcoder earns more fees or calls reward() later in that round, you can not access those rewards or fees. To really optimize, change your bonding status late in the round after most of the fees have been collected and reward() has been called.

• https://forum.livepeer.org/t/how-to-restart-a-transcoder-and-verify-its-active/342/2

  And the one mistake you can make is claiming earnings for the current round before reward is called, because then you miss the chance to re-allocate the rewards for the current round into your account.

If you think this report should be accepted given that context, I'd say it should at least have a reduced severity as it's at most an improvement proposal, not an inconsistency with the expected behavior.

[1] It would be great to have a way to prevent the double reporting practice on modified contracts when we run an audit.

[c4-sponsor]

victorges marked the issue as disagree with severity

[HickupHH3]

@victorges appreciate the context & info you've given!

I agree that at best this issue is QA(R).

[1] It would be great to have a way to prevent the double reporting practice on modified contracts when we run an audit.

This shouldn't be the case, I'll flag to the C4 team to tighten guidelines!