imsrybr0 - Broken invariant : the sum of all (delegateRewardsPerShare * delegated stake - reward debt) = the balance of the /x/bank AlloraPendingRewardForDelegatorAccountName module account when calculating reputer and delegator rewards

[sherlock-admin4]

imsrybr0

High

Broken invariant : the sum of all (delegateRewardsPerShare * delegated stake - reward debt) = the balance of the /x/bank AlloraPendingRewardForDelegatorAccountName module account when calculating reputer and delegator rewards

Note

I did not find this issue on my own, I only saw it being fixed on the main repository in this PR.

Summary

Broken invariant : the sum of all (delegateRewardsPerShare * delegated stake - reward debt) = the balance of the /x/bank AlloraPendingRewardForDelegatorAccountName module account when calculating reputer and delegator rewards.

Vulnerability Detail

When calculating reputer and delegators rewards :

• Delegate stakers rewards are based on what percentage of the total reputer's stakes the delegate stakes represent.
• The remaining rewards are sent to the reputer.

There are two issues in how this calculation is done : 1) The delegate reward per share is calculated base on the untrimmed total delegate reward, but only the trimmed total delegate reward end up being sent to the AlloraPendingRewardForDelegatorAccountName module account 2) The remaining rewards for the reputer are also calculated based the untrimmed total delegate rewards and will also be trimmed afterwards.

To illustrate this, let's assume we have a reputer that have equal stakes of 1uallo from the reputer itself and another delegate staker (50%/50%).

If the total rewards is 9 :

• Delegate staker rewards :
  • delegatorReward will be 4.5 (9 * 1uallo / 2uallo)
  • addShare will be 4.5 (delegatorReward / 1uallo)
  • AlloraPendingRewardForDelegatorAccountName balance will be 4
• Reputer rewards will be 4.5 (9 - 4.5), but only 4 will be sent to the reputer later

Impact

• Some rewards are not distributed (.i.e: in the example, 1 will be leftover)
• AlloraPendingRewardForDelegatorAccountName will be holding less than what is owed to delegate stakers

Code Snippet

GetRewardForReputerFromTotalReward

func GetRewardForReputerFromTotalReward(
    ctx sdk.Context,
    keeper keeper.Keeper,
    topicId uint64,
    reputerDelegatorRewards []types.TaskReward,
) ([]types.TaskReward, error) {

    var reputerRewards []types.TaskReward
    for _, reputerReward := range reputerDelegatorRewards {
        reputer := reputerReward.Address
        reward := reputerReward.Reward
        totalStakeAmount, err := keeper.GetStakeReputerAuthority(ctx, topicId, reputer)
        if err != nil {
            return nil, errors.Wrapf(err, "failed to get reputer stake")
        }
        // calculate reward for delegator total staked amount and send it to AlloraPendingRewardForDelegatorAccountName
        totalDelegatorStakeAmount, err := keeper.GetDelegateStakeUponReputer(ctx, topicId, reputer)
        if err != nil {
            return nil, errors.Wrapf(err, "failed to get reputer upon stake")
        }

        fraction := totalDelegatorStakeAmount.Mul(synth.CosmosIntOneE18()).Quo(totalStakeAmount)
        fractionUint, err := alloraMath.NewDecFromSdkInt(fraction)
        if err != nil {
            return nil, err
        }
        delegatorReward, err := reward.Mul(fractionUint)
        if err != nil {
            return nil, err
        }
        e18, err := alloraMath.NewDecFromSdkInt(synth.CosmosIntOneE18())
        if err != nil {
            return nil, err
        }
        delegatorReward, err = delegatorReward.Quo(e18)
        if err != nil {
            return nil, err
        }
        if delegatorReward.Gt(alloraMath.NewDecFromInt64(0)) {
            // update reward share
            // new_share = current_share + (reward / total_stake)
            totalDelegatorStakeAmountDec, err := alloraMath.NewDecFromSdkInt(totalDelegatorStakeAmount)
            if err != nil {
                return nil, err
            }
            addShare, err := delegatorReward.Quo(totalDelegatorStakeAmountDec) // <===== Audit : New share calculated using untrimmed delegatorReward
            if err != nil {
                return nil, err
            }
            currentShare, err := keeper.GetDelegateRewardPerShare(ctx, topicId, reputer)
            if err != nil {
                return nil, err
            }
            newShare, err := currentShare.Add(addShare)
            if err != nil {
                return nil, err
            }
            err = keeper.SetDelegateRewardPerShare(ctx, topicId, reputer, newShare)
            if err != nil {
                return nil, err
            }
            err = keeper.SendCoinsFromModuleToModule(
                ctx,
                types.AlloraRewardsAccountName,
                types.AlloraPendingRewardForDelegatorAccountName,
                sdk.NewCoins(sdk.NewCoin(params.DefaultBondDenom, delegatorReward.SdkIntTrim())), // <===== Audit : Only the trimmed delegatorReward is sent to the AlloraPendingRewardForDelegatorAccountName module account
            )
            if err != nil {
                return nil, errors.Wrapf(err, "failed to send coins to allora pend reward account")
            }
        }
        // Send remain rewards to reputer
        reputerRw, err := reward.Sub(delegatorReward) // <===== Audit : Remaining reputer reward is calculated using the non trimmed delegatorReward and will also be trimmed later.
        if err != nil {
            return nil, err
        }
        reputerRewards = append(reputerRewards, types.TaskReward{
            Address: reputerReward.Address,
            Reward:  reputerRw,
            TopicId: reputerReward.TopicId,
            Type:    types.ReputerAndDelegatorRewardType,
        })
    }

    return reputerRewards, nil
}

Tool used

Manual Review

Recommendation

Use the trimmed delegateRewards as basis for the calculation

Duplicate of #74

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Rewards are not correctly calculated

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/allora-network/allora-chain/pull/424