sherlock-audit / 2024-06-allora-judging

0 stars 0 forks source link

LZ_security - Reputer can DoS emissions/msgserver/InsertBulkReputerPayload #109

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

LZ_security

High

Reputer can DoS emissions/msgserver/InsertBulkReputerPayload

Summary

Malicious Reputer call InsertBulkReputerPayload ahead of time, let the other Reputer Scores of failure data.

Vulnerability Detail

InsertBulkReputerPayload passes through msg.ReputerValueBundles and verifies the signature of ReputerBundle,

but msg.sender is not validated, so anyone can call this function, but since bundle.Validate(), Reputer needs to sign the data or the call will fail.

If passed empty msg.ReputerValueBundles, the function returns a failure:

    if len(lossBundlesFromTopReputers) == 0 {
        return nil, types.ErrNoValidBundles
    }

But the problem is, if Reputer acts as an attack,Reputer can sign Bundles and validate them through bundle.Validate(),

If malicious Reputer yourself to signature of the bundle, put in a data to msg.ReputerValueBundles array, and then call InsertBulkReputerPayload, other Reputer bundle data will fail, because ReputerNonce is already used.

    reputerNonceUnfulfilled, err := ms.k.IsReputerNonceUnfulfilled(ctx, msg.TopicId, msg.ReputerRequestNonce.ReputerNonce)
    if err != nil {
        return nil, err
    }
    ......
    _, err = ms.k.FulfillReputerNonce(ctx, msg.TopicId, msg.ReputerRequestNonce.ReputerNonce)

The same problem applies to emissions/msgserver/InsertBulkWorkerPayload, A malicious Worker can call InsertBulkWorkerPayload.

Impact

Malicious reputer/worker invalidates the data of other reputer/workers

Code Snippet

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

https://github.com/sherlock-audit/2024-06-allora/blob/main/allora-chain/x/emissions/keeper/msgserver/msg_server_worker_payload.go#L219-L301

Tool used

Manual Review

Recommendation

Validate the msg.sender.

Duplicate of #88

sherlock-admin2 commented 3 months ago

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

sherlock-admin3 commented 3 months ago

Escalate

The issue here is different from issue #88. In issue 88, anyone could carry out the attack, but assuming issue 88 has been resolved by adding a whitelist, only the Reputer can call InsertBulkReputerPayload, and this problem still exists.

Let’s assume we have ReputerA, ReputerB, and ReputerC, with ReputerB and ReputerC submitting data through ReputerA. If ReputerB is an attacker, they can submit data with the same ReputerNonce as ReputerA before ReputerA submits their data. In this case, msg.ReputerValueBundles would only include ReputerB’s data, causing the data from ReputerA and ReputerC to become invalid.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

ZeroTrust01 commented 3 months ago

Escalate

The issue here is different from issue #88. In issue 88, anyone could carry out the attack, but assuming issue 88 has been resolved by adding a whitelist, only the Reputer can call InsertBulkReputerPayload, and this problem still exists.

Let’s assume we have ReputerA, ReputerB, and ReputerC, with ReputerB and ReputerC submitting data through ReputerA. If ReputerB is an attacker, they can submit data with the same ReputerNonce as ReputerA before ReputerA submits their data. In this case, msg.ReputerValueBundles would only include ReputerB’s data, causing the data from ReputerA and ReputerC to become invalid.

sherlock-admin3 commented 3 months ago

Escalate

The issue here is different from issue #88. In issue 88, anyone could carry out the attack, but assuming issue 88 has been resolved by adding a whitelist, only the Reputer can call InsertBulkReputerPayload, and this problem still exists.

Let’s assume we have ReputerA, ReputerB, and ReputerC, with ReputerB and ReputerC submitting data through ReputerA. If ReputerB is an attacker, they can submit data with the same ReputerNonce as ReputerA before ReputerA submits their data. In this case, msg.ReputerValueBundles would only include ReputerB’s data, causing the data from ReputerA and ReputerC to become invalid.

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 commented 3 months ago

Escalate

The issue here is different from issue #88. In issue 88, anyone could carry out the attack, but assuming issue 88 has been resolved by adding a whitelist, only the Reputer can call InsertBulkReputerPayload, and this problem still exists.

Let’s assume we have ReputerA, ReputerB, and ReputerC, with ReputerB and ReputerC submitting data through ReputerA. If ReputerB is an attacker, they can submit data with the same ReputerNonce as ReputerA before ReputerA submits their data. In this case, msg.ReputerValueBundles would only include ReputerB’s data, causing the data from ReputerA and ReputerC to become invalid.

88 describes a similar vulnerability with the same root cause as this report: the failure to validate msg.sender, which leads to DoS attacks in the InsertBulkReputerPayload/InsertBulkWorkerPayload functions. I would consider this a duplicate.

zhaojio commented 3 months ago

Escalate The issue here is different from issue #88. In issue 88, anyone could carry out the attack, but assuming issue 88 has been resolved by adding a whitelist, only the Reputer can call InsertBulkReputerPayload, and this problem still exists. Let’s assume we have ReputerA, ReputerB, and ReputerC, with ReputerB and ReputerC submitting data through ReputerA. If ReputerB is an attacker, they can submit data with the same ReputerNonce as ReputerA before ReputerA submits their data. In this case, msg.ReputerValueBundles would only include ReputerB’s data, causing the data from ReputerA and ReputerC to become invalid.

88 describes a similar vulnerability with the same root cause as this report: the failure to validate msg.sender, which leads to DoS attacks in the InsertBulkReputerPayload/InsertBulkWorkerPayload functions. I would consider this a duplicate.

The problem here is that even if the problem with msg.sender is not verified and fixed, the problem here still exists, so it is another problem.

WangSecurity commented 3 months ago

I agree with the lead judge that the root cause of the attack is actually the same. Moreover, the recommendations in both reports are the same. Hence, I believe the issues are with the same root cause, but with different attack paths. Planning to reject the escalation and leave the issue as it is.

WangSecurity commented 3 months ago

Result: High Duplicate of #88

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status: