Ethers.js: Error decoding bytes parameter for logs emitted by external functions (solc 0.4.x)

[cgewecke]

(Cross-reporting from web3 3544. Apologies in advance if this has already been reported here.)

Using the v5 ABI package, this error

Error: data out-of-bounds (length=36, offset=64, code=BUFFER_OVERRUN, version=abi/5.0.0-beta.153)
      at Logger.makeError (node_modules/@ethersproject/logger/lib/index.js:178:21)
      at Logger.throwError (node_modules/@ethersproject/logger/lib/index.js:187:20)
      ...

is thrown when decoding events with bytes params when

• contracts are compiled with solc 0.4.x
• emitting methods have external visibility

pragma solidity ^0.4.24;

contract A {

    event lockCallback(uint256 amount, uint256 allowance, bytes data);

    // Fine
    function public_fireEvent(bytes sig) public {
      emit lockCallback(5,5, sig);
    }
    // Errors
    function external_fireEvent(bytes sig) external {
        emit lockCallback(5,5, sig);
    }
}

Example data for contract above

public_fireEvent

0000000000000000000000000000000000000000000000000000000000000005
0000000000000000000000000000000000000000000000000000000000000005
0000000000000000000000000000000000000000000000000000000000000060
0000000000000000000000000000000000000000000000000000000000000004
abe985cb00000000000000000000000000000000000000000000000000000000

external_fireEvent

0000000000000000000000000000000000000000000000000000000000000028
0000000000000000000000000000000000000000000000000000000000000078
0000000000000000000000000000000000000000000000000000000000000060
0000000000000000000000000000000000000000000000000000000000000004
abe985cb

external's bytes data is not right-padded due to a bug reported in https://github.com/ethereum/solidity/issues/3493. The fix shipped with solc 0.5.x.

@ricmoo Do you have any views about what could/should be done for this case? 🙂

[ricmoo]

If this is only happening on events, the data can be sanitized by padding (right) with 0’s until the length is congruent to 0 mod 32 bytes.

I’ll have to think deeper whether this is safe to do in general though. It may be something that I could expose as part of the error recovery API...

[cgewecke]

the data can be sanitized by padding (right) with 0’s until the length is congruent to 0 mod 32 bytes

Multiple dynamic types in the event signature data seem to complicate this a little. For example with

event a(bytes8 b1, bytes b2, bytes b3)

solc 0.4.x data looks like:

public

00000000000000010000000000000000
00000000000000000000000000000000
00000000000000000000000000000000
00000000000000000000000000000060
00000000000000000000000000000000
000000000000000000000000000000a0
00000000000000000000000000000000
00000000000000000000000000000004
aaaaaaaa000000000000000000000000
00000000000000000000000000000000
00000000000000000000000000000000
00000000000000000000000000000004
bbbbbbbb000000000000000000000000
00000000000000000000000000000000

external

// Errors even when padded
0x
00000000000000010000000000000000
00000000000000000000000000000000
00000000000000000000000000000000
00000000000000000000000000000060
00000000000000000000000000000000
00000000000000000000000000000084
00000000000000000000000000000000
00000000000000000000000000000004
aaaaaaaa000000000000000000000000   
00000000000000000000000000000000  // ??  
00000004bbbbbbbb  

Have tried disabling the buffer overrun check in Reader#peekBytes and mis-formatted event data seems to decode ok for a range of inputs (including v. long bytes sequences).

[
 "0x0000000000000001",
 "0xaaaaaaaa",
 "0xbbbbbbbb"
]

Do you think a "tolerant" flag for abi.decode might be safe for events data generally?

[ricmoo]

I will have to look into this more, but I think this might indicate the data is absolutely corrupted and irrecoverable in a generic sense.

This makes things, I believe, undecidable.

Since there is no way to detect the bug, since one bytes may render the output congruent to 4 mod 32 and another bytes render the output to be congruent to 28 mod 32, the final result is now congruent to 0 mod 32.

There is no way to tell the first bytes is 4 long with a correct padding of 28 0’s vs. 4 bytes long with missing padding (the 28 following 0’s actually being the first 28 of the length of the next bytes)

There would need to be a way to flag an event as “handle-external-bug” for the event.

But now comes the bigger issue. If a contract used external and public functions with the same event, they could be mixed. Also, if an external caller a public with an emit, does it bug out or work? I guess that matters less, since an external can call and external using call (so usually to another contract).

Which means the output could literally have multiple valid (but different) interpretations. With no way to tell which is correct.

Also, it’s an np-complete algorithm, but the n is rather small, so that is less a concern to me.

The least worst option I can think of, would be adding another type, buggy-external- bytes. But this still won’t solve the above issues, and would require the developer defining the abi to take this into account...

Hmmmm...

[ricmoo]

Just to clarify my understanding. This means that even when the ethers AbiCoder accepted buggy events, it only worked if the bytes was the very last (left-to-right, depth-first evaluation) item in external method, right?

[cgewecke]

This means that even when the ethers AbiCoder accepted buggy events, it only worked if the bytes was the very last (left-to-right, depth-first evaluation) item in external method, right?

I'm honestly not sure.

In V5, with this Solidity (compiling with 0.4.24)

event E(bytes b1, bytes b2, uint u1); 

function fire(bytes b1, bytes b2, uint u1) external {
        emit E(b1,b2,u1);
}

The data looks like this:

00000000000000000000000000000000
00000000000000000000000000000060
00000000000000000000000000000000
00000000000000000000000000000084
00000000000000000000000000000000
00000000000000000000000000000001
00000000000000000000000000000000
00000000000000000000000000000004
aaaaaaaa000000000000000000000000
00000000000000000000000000000000
00000004bbbbbbbb

Disabling the buffer overflow error in Reader results in a successful decoding

[
 "0xaaaaaaaa",
 "0xbbbbbbbb",
 "1"
]

[cgewecke]

Hi, just circling back here...

My impression is that the consensus in the companion Web3 issue is that if there was a way to "sweep this under the rug" by allowing non-strict decoding of events (via a flag that disabled the buffer check), people would be ok with it.

The decoding seems to work in the identified cases without the overflow check and in general, event data is being formatted correctly by Solidity.

@ricmoo Do you have any thoughts about this?

[ricmoo]

After quite a bit of experimentation and playing around with possible scenarios, I have come to the conclusion that it seems safe to support events (and only events) using a "loose" Reader. Any extraneous bytes read from an adjacent (non-word aligned) ABI word will be discarded, and the offsets are always loaded from a base offset within each nest of the Coder, which in itself is popped off one the way back up the call stack (implicitly, since the base reader retains its location in the calling frame).

I have added support in 5.0.12, but only for the bytes and string types. All other dynamic types in ABIv1 have word-aligned sizes, so this is not an issue.

In Solidity 0.4 the ABIv2 was experimental, so any output with nested dynamic structures may contain the same issue, and are not supported as they may still trigger the OP issue. If someone out there has a strong need for support for ABIv2 nested structures in legacy Solidity external functions, please let me know, but I suspect no one was using this feature, and a lot more effort will be required to prove it is safe to process those with a loose Reader.

Please try this out and let me know if it works for you. I've tried it against the various examples opened up in all the issues I could find related to non-word-aligned bytes and string event data.

Thanks! :)

[cgewecke]

Thanks @ricmoo!

[zaman10d]

We Don't have Times to develope manage setting only we Wan setup automaticlly moving forward. If last behind