Web3.js: ABI objects passed into web3 functions may end up mutated

[haltman-at]

Expected behavior

ABI objects (or ABI item objects or ABI parameter objects) that are passed into web3 functions should not be mutated by web3, and should remain valid for use elsewhere without having to worry that web3 has altered them.

Actual behavior

If an ABI parameter object has type function, it will be mutated to have type bytes24 instead. As in, the actual object will be altered by a function that definitely shouldn't mutate it.

Steps to reproduce the behavior

const Web3 = require("web3");
const web3 = new Web3();
const type = { type: "function", name: "", internalType: "function () external", indexed: false };
web3.eth.abi.decodeParameter(type, '0x0000000000000000000000000000000000000000000000000000000000000000');
console.log(type);

You will observe that type has been mutated, and thus will produce incorrect results if used anywhere else.

Further commentary and suggested fix

The problem seems to have been introduced in PR #3586, which attempts to add support for parameters of type function, by translating them to bytes24. However, in doing so, it accidentally mutates the input. The problem can be seen on this line, which mutates type rather than creating a modified clone.

So, my suggested fix is to change that line to clone rather than mutate.

(The context here, btw, is that we're trying to update Truffle to use Web 1.3.0, and this issue is causing test failures in tests involving function ABI parameters.)

Environment

Node: v10.22.1
Web3: 1.3.0

[GregTheGreek]

Thank you for reporting this :)

[cgewecke]

@haltman-at

You will observe that type has been mutated, and thus will produce incorrect results if used anywhere else.

Could you describe what kind of incorrect results you're seeing?

[haltman-at]

The short version is that we've got an event which has a function as one of its parameters, and web3 alters the event's ABI item, with the result that it no longer has the correct selector. (The shorter result is that it doesn't really matter, because this should never happen in the first place!)

The slightly longer version is as follows. We have an event which has a function as one of its parameters. We run a transaction that emits this event. Web3 attempts to decode the events from the result, and in doing so, mutates the ABI for this event (note: I think this is where it happens but I haven't confirmed). (This results in web3 failing to decode the event, because now the selector is wrong, but I don't actually particularly care about that, because that's better than crashing, right?)

However we then attempt to decode the event ourselves using @truffle/decoder (which is what this was a test of) and now our decoding fails because again the selector was wrong. (And if somehow the selector were unaffected, it would still decode wrong, because it would then be decoded as a bytes24 instead of as a function; @truffle/decoder is capable of handling the latter and treats it differently from bytes24.)

So, that's the particular reason why our tests are failing, to whatever extent it's relevant.

[cgewecke]

@haltman-at Agree it shouldn't mutate the ABI - definitely wrong. The part I don't understand is this

This results in web3 failing to decode the event, because now the selector is wrong

Is it not the case that the decoding returns the expected result per the Solidity docs?, e.g

If external function types are used outside of the context of Solidity, they are treated as the function type, which encodes the address followed by the function identifier together in a single bytes24 type

[haltman-at]

A function is encoded like a bytes24, yes, but they're different types for the purposes of selector computation. In this particular case, for instance, the particular event is called CauseTrouble (because this was a test I expected to cause trouble :) ), and the function is its only input. So its signature is CauseTrouble(function), yielding a selector of 0x5809e1d35b22d18203bfda13a374dbb79900bee311bcb29adf6b24fee1570b2f.

However, changing the type to bytes24 results in a signature of CauseTrouble(bytes24) and a selector of 0xc75a514df15528f734fa677ecb00fed708ee1613f3e38c837855867fa8b1aa2e.

So, the event is emitted with the former selector. However, when web3 attempts to decode it -- which it does because we're sending the transaction with web3.eth.Contract (just realized I forgot to specify this, oops) -- the Contract object has no events with that selector; it only has an event with the latter, incorrect, selector.

So you're right that if web3 got as far as locating the specific ABI item to use in decoding, then yeah, it would decode it as a bytes24 just fine. The problem is that it doesn't get there, because due to the altered selector it can't locate the ABI item in the first place.

[cgewecke]

@haltman-at Ah yes I see! Thanks so much for explaining that!

[cgewecke]

@GregTheGreek

Is ok if I self-assign here & open a PR (with better tests) fixing this?

I am responsible for the errors #3586

[GregTheGreek]

@cgewecke sure! We're planning a release tongiht, but i can hold off. What do you think a timeline is on this?

[cgewecke]

@GregTheGreek Oh! I was thinking end of tomorrow?

I don't think the function ABI type is widely used in public facing Solidity methods since it's always crashed both Web3 and Ethers. Could go in the following release?

[GregTheGreek]

could always bump the rc candidate! If you get it in let me know?

[gnidan]

If you get a dist-tag out we can start testing the upgrade on Truffle's side

[gnidan]

Any updates on this? Thanks!

[GregTheGreek]

@cgewecke any news otherwise we can take this on

[cgewecke]

@GregTheGreek

Really sorry, this dropped through the cracks. Yes, if you'd like to fix this please go ahead.

Un-self-assigning...

[GregTheGreek]

No worries we'll get it done :)

[haltman-at]

Hi, do you mind if I pester you again about this? People are bugging us to upgrade, but we still need this fixed...

[haltman-at]

OK, I ended up putting up PR #3818 about this! I had trouble completing some of the checklist on my machine, but since this is so simple I hope that's OK.