sherlock-audit / 2024-06-allora-judging

0 stars 0 forks source link

pks_ - Malicious delegators can get more rewards than they should #102

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 2 months ago

pks_

High

Malicious delegators can get more rewards than they should

Summary

The RemoveDelegateStake and AddDelegateStake actions in the emissions module don't update the delegatedStakePlacement.RewardDebt to record the claimed reward after sending the pending reward to the delegators.

Vulnerability Detail

When delegators remove their stake, emissions keeper#RemoveDelegateStake calculate their pending reward then send the rewards to the delegators:

func (k *Keeper) RemoveDelegateStake(
    ...
    pendingReward, err = pendingReward.Sub(delegatedStakePlacement.RewardDebt)
    if err != nil {
        return err
    }
    if pendingReward.Gt(alloraMath.NewDecFromInt64(0)) {
        err = k.SendCoinsFromModuleToAccount(
            ctx,
            types.AlloraPendingRewardForDelegatorAccountName,
            delegator,
            sdk.NewCoins(sdk.NewCoin(params.DefaultBondDenom, pendingReward.SdkIntTrim())),
        )
        if err != nil {
            return errorsmod.Wrapf(err, "Sending pending reward to delegator failed")
        }
    }
    ....
}

However, the function don't update the delegatedStakePlacement.RewardDebt to record the claimed reward after sending the pending reward. This means that the delegators can claim the reward multiple times by removing their stake multiple times with little msg.amount every time.

Same issue also exists in AddDelegateStake action.

Impact

The delegators can get more rewards than they should.

Code Snippet

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/keeper/keeper.go#L974-L995

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/keeper/keeper.go#L782-L811

Tool used

vscode, Manual Review

Recommendation

Record the claimed reward after sending the reward to delegator as msg_server_stake#RewardDelegateStake did.

relyt29 commented 1 month ago

This is not a real bug. The reward debt is updated in that function, several lines below:

remove:

https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L1002-L1005 https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L1052

add:

https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L820-L827 https://github.com/sherlock-audit/2024-06-allora/blob/4e1bc73db32873476f8b0a88945815d3978d931c/allora-chain/x/emissions/keeper/keeper.go#L847

mystery0x commented 1 month ago

Invalid as reward debt is indeed updated