Minato7namikazi - Logic Bug in the `QuadrupleKeyCodec` Implementation

[sherlock-admin2]

Minato7namikazi

High

Logic Bug in the QuadrupleKeyCodec Implementation

Vulnerability Detail

in the QuadrupleKeyCodec implementation regarding how it handles nil values in the Encode and Decode methods.

The Issue

• Encode: When encoding a Quadruple key, if one of the key parts (e.g., key.k2) is nil, the code directly proceeds to encode the next key part using the previous writtenTotal. This means that no space is reserved in the buffer for the nil key part, leading to incorrect encoding.

• Decode: Similarly, during decoding, if a key part is nil, the code still attempts to decode the subsequent parts, potentially leading to data misalignment and incorrect key reconstruction.

impact

This bug can lead to data corruption and inconsistencies in the underlying storage. It may cause:

• Incorrect Key Retrieval: When trying to retrieve a value from the store using a key that has a nil part, the decoding might fail, or the wrong value might be returned.
• Key Collisions: Incorrect encoding could lead to multiple logically different keys being encoded into the same byte representation, resulting in key collisions.
• Data Loss: In some cases, the misalignment of data during decoding could lead to data being overwritten or lost.

• Code Snippet

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

Tool used

Manual Review

Recommendation

we need to modify the Encode and Decode methods to correctly handle nil key parts. ensures that the encoded representation of the Quadruple key accurately reflects the presence or absence of each key part.

PoC

package keeper_test

import (
    "fmt"
    "testing"

    "github.com/stretchr/testify/assert"
    "github.com/cosmos/cosmos-sdk/collections/codec"
    "github.com/allora-network/allora-chain/x/emissions/keeper"
)

func TestQuadrupleKeyCodecNilValues(t *testing.T) {
    // Create sample key codecs for each part of the Quadruple
    stringKeyCodec := codec.StringKeyCodec{}
    uint64KeyCodec := codec.Uint64KeyCodec{}
    boolKeyCodec := codec.BoolKeyCodec{}

    // Create a QuadrupleKeyCodec
    quadrupleKeyCodec := keeper.QuadrupleKeyCodec(stringKeyCodec, uint64KeyCodec, boolKeyCodec, stringKeyCodec) // Note the nil value for k3

    // Create a Quadruple key with a nil value in the third part
    key := keeper.Join4("part1", uint64(123), nil, "part4") 

    // Encode the key
    encoded, err := quadrupleKeyCodec.Encode(nil, key)
    assert.NoError(t, err)

    // Decode the key
    _, decoded, err := quadrupleKeyCodec.Decode(encoded)
    assert.NoError(t, err)

    // Assertion to demonstrate the incorrect decoding
    expectedDecoded := keeper.Join4("part1", uint64(123), nil, "part4") // The third part should still be nil
    assert.Equal(t, expectedDecoded, decoded, "Decoded key does not match the original key") 
}

Explanation

1. Creates basic key codecs for strings, uint64, and booleans.
2. Creates a QuadrupleKeyCodec using the sample codecs.
3. Creates a Quadruple key where the third part (k3) is intentionally set to nil.
4. Encodes the key using the QuadrupleKeyCodec.
5. Decodes the encoded bytes back into a Quadruple key.
6. Asserts that the decoded key is equal to the original key, including the nil value in the third part. This assertion should fail with the original buggy QuadrupleKeyCodec implementation.

Expected Output

The test should initially fail due to the incorrect handling of the nil value in the original QuadrupleKeyCodec.Encode and QuadrupleKeyCodec.Decode methods.

[relyt29]

This is an intellectually interesting bug however I have a few problems with it, and would like further justification from the submitter before I can acknowledge this to be a real bug:

1. The implementation of quadruple.go is forked from the collections triple.go, so if the claimed error is present it should also be present on cosmos-sdk
2. I cannot reproduce the bug as submitted in the PoC. Was this written with something like ChatGPT? The imports are wrong, the size of the buffer passed to Encode is nil so an error would occur when trying to call it, and even if you corrected those issues, I am not able to find a way to compile the repo with a nil value passed to Join4 - I get a compiler error CannotInferTypeArgs

That said I can add a PR that throws an error from Encoding if any component is nil: https://github.com/allora-network/allora-chain/pull/467

[sherlock-admin2]

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

[mystery0x]

request poc

[sherlock-admin4]

PoC requested from @Minato7namikazi

Requests remaining: 6

[relyt29]

After digging some into the failing tests, it appears encoding nil values is a necessary feature if you want to support nil rangers that will iterate over all keys

I'm inclined to call this a wontfix until theres some substantiated bug that can actually clearly demonstrate a problem