LZ_security - Malicious Reputer cause emissions/msgserver/InsertBulkReputerPayload to fail

[sherlock-admin4]

LZ_security

High

Malicious Reputer cause emissions/msgserver/InsertBulkReputerPayload to fail

Summary

Malicious Reputer passing in abnormal data causes InsertBulkReputerPayload to fail.

Vulnerability Detail

The synth.CalcNetworkLosses function computes all data in bundles, InsertBulkReputerPayload -> synth.CalcNetworkLosses -> RunningWeightedAvgUpdate

func RunningWeightedAvgUpdate(
    runningWeightedAvg *RunningWeightedLoss,
    nextWeight Weight,
    nextValue Weight,
) (RunningWeightedLoss, error) {
@>  nextValTimesWeight, err := nextValue.Mul(nextWeight)
    if err != nil {
        return RunningWeightedLoss{}, err
    }
    newUnnormalizedWeightedLoss, err := runningWeightedAvg.UnnormalizedWeightedLoss.Add(nextValTimesWeight)
    if err != nil {
        return RunningWeightedLoss{}, err
    }
    newSumWeight, err := runningWeightedAvg.SumWeight.Add(nextWeight)
    if err != nil {
        return RunningWeightedLoss{}, err
    }
    return RunningWeightedLoss{
        UnnormalizedWeightedLoss: newUnnormalizedWeightedLoss,
        SumWeight:                newSumWeight,
    }, nil
}

nextValue * nextWeight If the value exceeds the maximum value of unit128, a failure is returned. As a result, InsertBulkReputerPayload fails.

    networkLossBundle, err := synth.CalcNetworkLosses(stakesByReputer, bundles, params.Epsilon)
    if err != nil {
        return nil, err
    }

Malicious reputers can pass in a larger CombinedValue(nextValue) and it will return an error, bundles array stores data from a number of Reputer. Execution will fail if one of the Reputer is malicious.

The following test code demonstrates CalcNetworkLosses execution failure:

func getTestCasesTwoWorkers() []struct {
    name            string
    stakesByReputer map[inference_synthesis.Worker]cosmosMath.Int
    reportedLosses  emissions.ReputerValueBundles
    epsilon         alloraMath.Dec
    expectedOutput  emissions.ValueBundle
    expectedError   error
} {
    return []struct {
        name            string
        stakesByReputer map[inference_synthesis.Worker]cosmosMath.Int
        reportedLosses  emissions.ReputerValueBundles
        epsilon         alloraMath.Dec
        expectedOutput  emissions.ValueBundle
        expectedError   error
    }{
        {
            name: "simple two reputer combined loss",
            stakesByReputer: map[inference_synthesis.Worker]cosmosMath.Int{
                "worker1": inference_synthesis.CosmosIntOneE18(),           // 1 token
                "worker2": inference_synthesis.CosmosIntOneE18().MulRaw(2), // 2 tokens
            },
            reportedLosses: emissions.ReputerValueBundles{
                ReputerValueBundles: []*emissions.ReputerValueBundle{
                    {
                        ValueBundle: &emissions.ValueBundle{
                            Reputer:       "worker1",
                            CombinedValue: alloraMath.MustNewDecFromString("1e99999"),  //@audit The attacker passes in abnormal data
                            NaiveValue:    alloraMath.MustNewDecFromString("0.1"),
                            InfererValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                            },
                            ForecasterValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                            },
                            OneOutInfererValues: []*emissions.WithheldWorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                            },
                            OneOutForecasterValues: []*emissions.WithheldWorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                            },
                            OneInForecasterValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.1"),
                                },
                            },
                        },
                    },
                    {
                        ValueBundle: &emissions.ValueBundle{
                            Reputer:       "worker2",
                            CombinedValue: alloraMath.MustNewDecFromString("0.2"),
                            NaiveValue:    alloraMath.MustNewDecFromString("0.2"),
                            InfererValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                            },
                            ForecasterValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                            },
                            OneOutInfererValues: []*emissions.WithheldWorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                            },
                            OneOutForecasterValues: []*emissions.WithheldWorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                            },
                            OneInForecasterValues: []*emissions.WorkerAttributedValue{
                                {
                                    Worker: "worker1",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                                {
                                    Worker: "worker2",
                                    Value:  alloraMath.MustNewDecFromString("0.2"),
                                },
                            },
                        },
                    },
                },
            },
            epsilon: alloraMath.MustNewDecFromString("1e-4"),
            expectedOutput: emissions.ValueBundle{
                CombinedValue: alloraMath.MustNewDecFromString("0.166666666"),
                NaiveValue:    alloraMath.MustNewDecFromString("0.166666666"),
                InfererValues: []*emissions.WorkerAttributedValue{
                    {
                        Worker: "worker1",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                    {
                        Worker: "worker2",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                },
                ForecasterValues: []*emissions.WorkerAttributedValue{
                    {
                        Worker: "worker1",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                    {
                        Worker: "worker2",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                },
                OneOutInfererValues: []*emissions.WithheldWorkerAttributedValue{
                    {
                        Worker: "worker1",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                    {
                        Worker: "worker2",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                },
                OneOutForecasterValues: []*emissions.WithheldWorkerAttributedValue{
                    {
                        Worker: "worker1",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                    {
                        Worker: "worker2",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                },
                OneInForecasterValues: []*emissions.WorkerAttributedValue{
                    {
                        Worker: "worker1",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                    {
                        Worker: "worker2",
                        Value:  alloraMath.MustNewDecFromString("0.166666666"),
                    },
                },
            },
            expectedError: nil,
        },
    }
}

func (s *InferenceSynthesisTestSuite) TestCalcNetworkLosses2() {
    tests := getTestCasesTwoWorkers()
    //require := s.Require()
    for _, tc := range tests {
        s.Run(tc.name, func() {
            output , err := inference_synthesis.CalcNetworkLosses(tc.stakesByReputer, tc.reportedLosses, tc.epsilon)
            fmt.Println(err)
            fmt.Println(output)
        })
    }
}

Put the test code into the test file: allora-chain/x/emissions/keeper/inference_synthesis/network_losses_test.go

cd allora-chain/x/emissions/keeper/inference_synthesis/ go test -v -run TestModuleTestSuite/TestCalcNetworkLosses2

Error updating running weighted average for next combined loss: decimal multiplication error: exponent out of range [/home/zhaojie/hacks/2024-06-allora-zhaojio/allora-chain/math/dec.go:269]
{0 <nil>  [] 0 [] [] 0 [] [] []}
-

Impact

InsertBulkReputerPayload fails to be executed. As a result, data of the Reputers cannot be submitted

Code Snippet

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/keeper/msgserver/msg_server_losses.go#L176-L179

Tool used

Manual Review

Recommendation

Perform security checks on data submitted by Reputers.

[sherlock-admin4]

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

0xmystery commented:

Intended behavior

[ZeroTrust01]

Escalate

This issue is valid.

A malicious Reputer uses abnormal parameters to cause the ReputerPayload to fail (panic) during addition, which is not the intended behavior.

[sherlock-admin3]

Escalate

This issue is valid.

A malicious Reputer uses abnormal parameters to cause the ReputerPayload to fail (panic) during addition, which is not the intended behavior.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

Escalate

This issue is valid.

A malicious Reputer uses abnormal parameters to cause the ReputerPayload to fail (panic) during addition, which is not the intended behavior.

@ZeroTrust01 InsertBulkReputerPayload wont panic during addition. it instead handles the error by returning it. This should be intended behavior.

[zhaojio]

Escalate This issue is valid. A malicious Reputer uses abnormal parameters to cause the ReputerPayload to fail (panic) during addition, which is not the intended behavior.

@ZeroTrust01 InsertBulkReputerPayload wont panic during addition. it instead handles the error by returning it. This should be intended behavior.

The problem is that the function fails to execute, which becomes a DoS attack.

[WangSecurity]

In that case, InsertBulkReputerPayload will fail forever or it's only a one-block DOS? Excuse me if it's a silly question.

[zhaojio]

In that case, InsertBulkReputerPayload will fail forever or it's only a one-block DOS? Excuse me if it's a silly question.

1. After the attack, if ReputerRequestNonce used by others(or attacker), InsertBulkReputerPayload would always fail.
2. If in the next block the caller is still using the same parameter called InsertBulkReputerPayl oad, still fail, because contain the data of the attacker poisoned.
3. An attacker can always use the same method to attack.

[WangSecurity]

Thank you for the clarification. Additional small question, the function is still callable, the Reputer has to use a different nonce (ReputerRequestNonce) and a different payload, correct? Only that nonce and the same payload are DOSed?

Also, for example, can the reputer call this very same function with the same payload but with a different nonce, or vice versa? Or both have to be different from what the attacker used?

[zhaojio]

Thank you for the clarification. Additional small question, the function is still callable, the Reputer has to use a different nonce (ReputerRequestNonce) and a different payload, correct? Only that nonce and the same payload are DOSed?

Also, for example, can the reputer call this very same function with the same payload but with a different nonce, or vice versa? Or both have to be different from what the attacker used?

The Nonce is generated based on the BlockHeight. If the Nonce is used, it cannot be used again. payload contains WorkNonce. WorkNonce and RequestNonce are associated. payload and nonce cannot be replaced with each other, the new payload requires a new nonce. The nonce is an important resource of the system.

[WangSecurity]

Then, I'm not sure I understand what the problem is. The report says that the malicious reputer uses a large CombinedValue(nextValue) which results in failure of InsertBulkReputerPayload with the same nonce and payload. But, the same nonce shouldn't be used again in the first place. As I understand the issue is only a 1-block DOS. Hence, should remain invalid, planning to reject the escalation.

[ZeroTrust01]

Then, I'm not sure I understand what the problem is. The report says that the malicious reputer uses a large CombinedValue(nextValue) which results in failure of InsertBulkReputerPayload with the same nonce and payload. But, the same nonce shouldn't be used again in the first place. As I understand the issue is only a 1-block DOS. Hence, should remain invalid, planning to reject the escalation.

The issue is not a 1-block DOS. The attacker can use the same method to attack on every block.

[WangSecurity]

But, in that case, the attacker has to perpetually execute an attack in each block to cause permanent DOS, correct? If we take one instance of the attack it's only a one-block DOS, correct? Moreover, as I understand this function is not time-sensitive. Hence, I believe the following rule from DOS requirements apply:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Hence, planning to reject the escalation and leave the issue as it is, unless anything from the above is incorrect.

[zhaojio]

But, in that case, the attacker has to perpetually execute an attack in each block to cause permanent DOS, correct? If we take one instance of the attack it's only a one-block DOS, correct? Moreover, as I understand this function is not time-sensitive. Hence, I believe the following rule from DOS requirements apply:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Hence, planning to reject the escalation and leave the issue as it is, unless anything from the above is incorrect.

But the problem here has nothing to do with gas, and there is no need to frontrunning, the input parameters are not checked in the code, and the attack will cause the function to fail to execute normally, which is inconsistent with the wishes of the caller.

[zhaojio]

The attacker does not consume gas, the attacker just provides a harmful data. The attacker has no cost and can attack every block.

[WangSecurity]

I see that the issue doesn't require front-running, but the impact here is that InsertBulkReputerPayload is DOSed only for one block. Hence, the rule applies, because for this issue to have constant DOS, the attack has to be repeated in the every block. Thus, it's considered a one-block DOS.

Moreover, as I understand the report doesn't talk about the impact of harmful data, but about InsertBulkReputerPayload being unable to call for only one block.

Hence, I still stand by my previous decision, planning to reject the escalation and leave the issue as it is.

[zhaojio]

I see that the issue doesn't require front-running, but the impact here is that InsertBulkReputerPayload is DOSed only for one block. Hence, the rule applies, because for this issue to have constant DOS, the attack has to be repeated in the every block. Thus, it's considered a one-block DOS.

Moreover, as I understand the report doesn't talk about the impact of harmful data, but about InsertBulkReputerPayload being unable to call for only one block.

Hence, I still stand by my previous decision, planning to reject the escalation and leave the issue as it is.

impact has been explained in the report:

InsertBulkReputerPayload fails to be executed. As a result, data of the Reputers cannot be submitted.

If the caller using the same parameters, InsertBulkReputerPayload will always call you don't succeed.

[WangSecurity]

impact has been explained in the report

Yeah, sorry for the confusion, I didn't mean the impact is not in the report but meant that the impact in the report is only one-block DOS.

If the caller using the same parameters, InsertBulkReputerPayload will always call you don't succeed

But, as I understand from the protocol's design, they shouldn't be able to call this function with the same parameters. For example, nonce by definition is the number that is expected to be used only once.

Hence, the decision remains the same. Planning to reject the escalation and leave the issue as it is.

[zhaojio]

they shouldn't be able to call this function with the same parameters.

If the function call succeeds, it cannot be called again with the same parameters. But if an attacker causes the function call to fail, the caller cannot call it again with the same parameters, so it's not one-block DOS.

[WangSecurity]

Oh, I see, excuse me for the confusion. But, one iteration of the attack blocks only one set of the parameters. Hence, I would still say it’s low severity.

[zhaojio]

Oh, I see, excuse me for the confusion. But, one iteration of the attack blocks only one set of the parameters. Hence, I would still say it’s low severity.

Why low severity, because the caller modifies the parameter to avoid the attack?

The caller of this function is b7s node, It is not a concept similar to an EOA account, therefore, the caller cannot modify the parameters to avoid the attack.

    // Make 1 request per worker
@>  req := &emissionstypes.MsgInsertBulkReputerPayload{
        Sender: ap.Address,
        ReputerRequestNonce: &emissionstypes.ReputerRequestNonce{
            ReputerNonce: nonceCurrent,
            WorkerNonce:  nonceEval,
        },
        TopicId:             topicId,
        ReputerValueBundles: valueBundles,
    }
    // Print req as JSON to the log
    reqJSON, err := json.Marshal(req)
    if err != nil {
        ap.Logger.Error().Err(err).Msg("Error marshaling MsgInsertBulkReputerPayload to print Msg as JSON")
    } else {
        ap.Logger.Info().Str("req_json", string(reqJSON)).Msg("Sending Reputer Mode Data")
    }

    go func() {
@>        _, _ = ap.SendDataWithRetry(ctx, req, NUM_REPUTER_RETRIES, NUM_REPUTER_RETRY_MIN_DELAY, NUM_REPUTER_RETRY_MAX_DELAY, "Send Reputer Leader Data")
    }()

[WangSecurity]

I didn’t say anything about avoiding the attack. I’ve said this is low severity because one iteration of the attack blocks only one set of values. The function can be called with other nonces and payloads. Hence, I believe it’s low severity. The decision remains the same, planning to reject the escalation and leave the issue as it is.

[zhaojio]

I didn’t say anything about avoiding the attack. I’ve said this is low severity because one iteration of the attack blocks only one set of values. The function can be called with other nonces and payloads. Hence, I believe it’s low severity. The decision remains the same, planning to reject the escalation and leave the issue as it is.

Failure of payloads submission will result in failure of weight update, calculation of reward is based on weight, resulting in loss of funds, it not low severity.

[WangSecurity]

That's what I was missing I think. To clarify, Reputer's data affects weights which affect rewards. Hence, if the attacker executes this attack with Nonce 1 and payload XYZ (arbitrary examples), the transaction fails, but these values cannot be used again.

In that case, payload XYZ will be forever lost and not applied to weights update and rewards distribution. Correct?

To clarify, everyone can become a reputer, correct? And it's not a trusted role?

[zhaojio]

In that case, payload XYZ will be forever lost and not applied to weights update and rewards distribution. Correct?

payload XYZ wasn't lost, but couldn't be recommitted(same as forever lost), as explained in this comment https://github.com/sherlock-audit/2024-06-allora-judging/issues/112#issuecomment-2297858195

To clarify, everyone can become a reputer, correct? And it's not a trusted role?

Reputers are participants in the system,I think it's open to anyone, not trusted roles like admin. https://docs.allora.network/devs/reputers/reputers

[relyt29]

I think this is a legitimate bug, medium severity - the problem is that you should not be able to deny other reputers rewards for their work that block, even if your reputation data is bogus.

Fixed in https://github.com/allora-network/allora-chain/pull/494 - we removed blockless altogether, there is no aggregation step of reputer data now, each reputer separately sends a transaction to the network to upload their data so one reputer cannot deny other reputer's rewards

yes this is a one-block DoS but I still consider it quite serious

[WangSecurity]

Excuse me for the confusion and not understanding the impact fully. Indeed by submitting the payload with a high next value, the payload will fail to submit and cannot be submitted later. This leads to a loss of rewards. The high severity is appropriate since it leads to a loss of funds without extensive limitations. Planning to accept the escalation and validate with high severity.

[WangSecurity]

Result: High Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZeroTrust01: accepted

[WangSecurity]

@mystery0x @zhaojio there are no duplicates, correct?

[sherlock-admin2]

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