Minato7namikazi - Inconsistent Reward Calculation and Distribution in reputer_rewards.go

[sherlock-admin2]

Minato7namikazi

High

Inconsistent Reward Calculation and Distribution in reputer_rewards.go

Vulnerability Detail

The issue arises in the interaction between GetRewardPerReputer and GetRewardForReputerFromTotalReward.

1. GetRewardPerReputer: This function calculates rewards for both reputers and their delegators using the reputerFraction and totalReputerRewards. It creates TaskReward entries where Reward contains the combined reward for both the reputer and their delegators.

2. GetRewardForReputerFromTotalReward: This function is meant to separate the delegator's reward and send it to a pending account. However, it takes the reputerDelegatorRewards (which contain the combined rewards) and tries to calculate the delegator's portion again. This can lead to the delegator being over-rewarded.

Consequences:

• Delegators Over-Rewarded: The delegator's reward is calculated twice (once in GetRewardPerReputer and again in GetRewardForReputerFromTotalReward). This can lead to a significant discrepancy in the reward distribution, as delegators will receive more than their fair share.
• Reputers Under-Rewarded: Conversely, reputers may receive less than their intended reward due to the excess allocation to delegators.

Code Snippet

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/module/rewards/reputer_rewards.go#L233

Tool used

Manual Review

Recommendation

Separate Reward Calculation**

The solution is to separate the reward calculation for reputers and delegators in GetRewardPerReputer. Here's the improved code:

// ... other functions ...

// Get reward per reputer based on total reputer rewards and reputer fractions
// W_im = w_ij * W_i
func GetRewardPerReputer(
    ctx sdk.Context,
    keeper keeper.Keeper,
    topicId uint64,
    totalReputerRewards alloraMath.Dec,
    reputerAddresses []string,
    reputersFractions []alloraMath.Dec,
) ([]types.TaskReward, error) {
    var reputerRewards []types.TaskReward

    for i, reputerFraction := range reputersFractions {
        reputerReward, err := reputerFraction.Mul(totalReputerRewards)
        if err != nil {
            return nil, errors.Wrapf(err, "failed to calculate reputer rewards")
        }
        reputerRewards = append(reputerRewards, types.TaskReward{
            Address: reputerAddresses[i],
            Reward:  reputerReward,
            TopicId: topicId,
            Type:    types.ReputerRewardType,
        })
    }

    // Calculate and send delegator rewards separately
    var delegatorRewards []types.TaskReward
    for i, reputerFraction := range reputersFractions {
        // Fetch delegator stake and calculate their portion
        totalDelegatorStakeAmountInt, err := keeper.GetDelegateStakeUponReputer(ctx, topicId, reputerAddresses[i])
        if err != nil {
            return nil, errors.Wrapf(err, "failed to get reputer upon stake")
        }
        // ... (rest of the delegator reward calculation logic - similar to what you have in GetRewardForReputerFromTotalReward, but now it's only for the delegator portion)
        // ...

        delegatorRewards = append(delegatorRewards, types.TaskReward{ /* ... */ })
    }

    // Send delegator rewards 
    // ... (same as in GetRewardForReputerFromTotalReward) ...

    return reputerRewards, nil
}

Key Changes:

• We now have two loops in GetRewardPerReputer. The first calculates the reputer's rewards and the second calculates the delegator's rewards.
• TaskReward Types:
  • The reputerRewards have a type of ReputerRewardType.
  • The delegatorRewards (calculated in the second loop) would have a type of DelegatorRewardType.
• The call to GetRewardForReputerFromTotalReward is removed as it's no longer needed.

Benefits:

The reward calculation and distribution logic is now correct and fair to both reputers and delegators.

PoC

package main

import (
    "fmt"

    "github.com/shopspring/decimal" // Using decimal library for arbitrary precision
)

// Simulate types
type TaskReward struct {
    Address string          
    Reward  decimal.Decimal 
    TopicId uint64          
}

func getRewardPerReputer(totalReputerRewards decimal.Decimal, reputerAddresses []string, reputersFractions []decimal.Decimal) []TaskReward {
    var reputerRewards []TaskReward
    for i, fraction := range reputersFractions {
        reward := fraction.Mul(totalReputerRewards)
        reputerRewards = append(reputerRewards, TaskReward{reputerAddresses[i], reward, 1}) // Assume topicId = 1
    }
    return reputerRewards
}

func getRewardForReputerFromTotalReward(reputerDelegatorRewards []TaskReward) []TaskReward {
    // Simplified: Simulate calculating delegator reward (which should be separate)
    var reputerRewards []TaskReward
    for _, reward := range reputerDelegatorRewards {
        reputerRewards = append(reputerRewards, TaskReward{reward.Address, reward.Reward.Mul(decimal.NewFromFloat(0.8)), 1}) // Assume 80% for reputer
    }
    return reputerRewards
}

func main() {
    totalReputerRewards := decimal.NewFromInt(1000) // 1000 units of reward
    reputerAddresses := []string{"reputer1", "reputer2"}
    reputersFractions := []decimal.Decimal{decimal.NewFromFloat(0.6), decimal.NewFromFloat(0.4)}

    reputerDelegatorRewards := getRewardPerReputer(totalReputerRewards, reputerAddresses, reputersFractions)
    fmt.Println("Reputer/Delegator Combined Rewards:", reputerDelegatorRewards)

    // Incorrectly using combined rewards again
    reputerRewards := getRewardForReputerFromTotalReward(reputerDelegatorRewards)
    fmt.Println("Incorrectly Calculated Reputer Rewards:", reputerRewards) 
}

Explanation

1. We create simple structs and use the decimal library for precision.
2. Calculates combined rewards for reputers and delegators.
3. Simulates the incorrect logic by trying to calculate the reputer's portion again from the combined reward. (In the original code, it would attempt to calculate and send delegator rewards here).
4. Sets up a scenario and demonstrates the incorrect calculation.

Output: The bug becomes evident in the output:

Reputer/Delegator Combined Rewards: [{reputer1 600 1} {reputer2 400 1}]
Incorrectly Calculated Reputer Rewards: [{reputer1 480 1} {reputer2 320 1}]

You'll notice that in the second output, the values are 80% of the original combined rewards. This means the delegator's portion was applied twice.

test_suite integration

package rewards_test

import (
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/shopspring/decimal" // Using decimal library for arbitrary precision
)

// Simulate types
type TaskReward struct {
    Address string          
    Reward  decimal.Decimal
    TopicId uint64          
}

func getRewardPerReputer(totalReputerRewards decimal.Decimal, reputerAddresses []string, reputersFractions []decimal.Decimal) []TaskReward {
    // ... (same as in the previous PoC)
}

func getRewardForReputerFromTotalReward(reputerDelegatorRewards []TaskReward) []TaskReward {
    // ... (same as in the previous PoC - simulating the bug)
}

func TestGetReputersRewards(t *testing.T) {
    totalReputerRewards := decimal.NewFromInt(1000)
    reputerAddresses := []string{"reputer1", "reputer2"}
    reputersFractions := []decimal.Decimal{decimal.NewFromFloat(0.6), decimal.NewFromFloat(0.4)}

    reputerDelegatorRewards := getRewardPerReputer(totalReputerRewards, reputerAddresses, reputersFractions)

    // Expected combined rewards
    expectedCombinedRewards := []TaskReward{
        {"reputer1", decimal.NewFromInt(600), 1},
        {"reputer2", decimal.NewFromInt(400), 1},
    }
    assert.Equal(t, expectedCombinedRewards, reputerDelegatorRewards, "Combined rewards should be calculated correctly")

    reputerRewards := getRewardForReputerFromTotalReward(reputerDelegatorRewards)

    // Expected reputer rewards (after fixing the bug, these would be different)
    expectedReputerRewards := []TaskReward{
        {"reputer1", decimal.NewFromInt(480), 1}, // Currently incorrect due to bug
        {"reputer2", decimal.NewFromInt(320), 1}, // Currently incorrect due to bug
    }
    assert.Equal(t, expectedReputerRewards, reputerRewards, "Reputer rewards are incorrect due to double-counting delegator reward") 
}

[relyt29]

The delegator reward is a fraction of the total reward for that reputer and all their delegators combined (reputerDelegatorRewards)

it doesn't re-calculate the delegator reward. It takes a fraction off of the total reward for that reputer and all their delegates, and then sends off the delegators share

[mystery0x]

Delegator rewards are correctly calculated