The `indexed` Keyword in Events Causes Data Loss for String Variables

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0x36910b7540d379a44c220280d93eb7c8da5af77d853f7d3c04659deee5c65f60 Severity: low

Description: Description\

when the indexed keyword is used for reference type variables such as dynamic arrays or strings, it will return the hash of the mentioned variables. Thus, the event which is supposed to inform all of the applications subscribed to its emitting transaction (e.g. front-end of the DApp, or the backend listeners to that event), would get a meaningless and obscure 32 bytes that correspond to keccak256 of an encoded dynamic array. This may cause some problems on the DApp side and even lead to data loss. For more information about the indexed events, check here:

(https://docs.soliditylang.org/en/v0.8.17/abi-spec.html?highlight=indexed#events)

The problem exists inside the BaseManagedNFTStrategyUpgradeable contract. The event SetName is defined in such a way that the string variable of name is indexed. The function setName() is intended to allow the admin to update the strategy's name for clarity or rebranding purposes. However, with the current design, the expected parameter wouldn't be emitted properly and front-end would get a meaningless one-way hash.

Attachments

Proof of Concept (PoC) File Consider this scenario as an example:

1. The function setName() is called by the admin
2. Inside the function setName() we expect to see the the string of "name" be emitted
3. But as the event topic is defined as indexed we'll get an obscure 32-byte hash and listeners will not be notified properly. Thus, the name would be lost in the DApp.

For test purposes, one can run this test file:

    event SetName(string indexed newName);
    event SetName1(string newName);

    function test_emitter() public {

        string memory _name = "Sample Name";

        emit SetName(_name);
        emit SetName1(_name);
    }

Outputs of test:

SetName event:

[
    {
        "from": "0xdaf08126327C70504022e1DA364F5b1057b0B0ed",
        "topic": "0x4df9dcd34ae35f40f2c756fd8ac83210ed0b76d065543ee73d868aec7c7fcf02",
        "event": "SetName",
        "args": {
            "0": {
                "_isIndexed": true,
                "hash": "0xa094d9991fb6142a2d0dd7fe3e3c620a753d2e744ad6971a57523fd99381b28d"
            }
        }
    }

SetName1 event:

    {
        "from": "0xdaf08126327C70504022e1DA364F5b1057b0B0ed",
        "topic": "0xf458a30041a3a6ccca7067a8e08f990339b56b230ad983a14221a60e71e45b18",
        "event": "SetName1",
        "args": {
            "0": "Sample Name",
            "newName": "Sample Name"
        }
    }
]

As you can see, the intended outputs wouldn't be emitted if the indexed keyword is used for the name (string), and instead, the hash of the name is returned.

2. Revised Code File (Optional)

Consider modifying the event setName inside the contract IManagedNFTStrategy :

-   event SetName(string indexed newName);
+   event SetName(string newName);

[0xmahdirostami]

yes, thank you.

valid.