Converting an invalid number into an enum in an argument doesn't return a 0x21 panic code

[fvictorio]

Description

When an uint with an invalid value is converted into an enum, a 0x21 panic code is returned. But if the enum is a parameter of a function, this does not happen.

Environment

• Compiler version: 0.8.17
• Target EVM version (as per compiler settings): London
• Framework/IDE (e.g. Truffle or Remix): Hardhat
• EVM execution environment / backend / blockchain client: Hardhat Network
• Operating system: Ubuntu

Steps to Reproduce

Deploy this contract:

enum MyEnum {
  A, B
}

contract Foo {
  function f(MyEnum x) public {
  }

  function g(uint x) public {
    MyEnum myEnum = MyEnum(x);
  }
}

And call f and g with 2. g produces a 0x21 panic code, while f doesn't.

[leonardoalt]

~Afaik this check is part of ABI decoding only, so casts and enum constructors will not generate the panic, so the current behavior is by design as I see it.~

Edit: I misunderstood the issue at first, disregard this.

[0xalpharush]

+1 It would be nice to have ABI decoding have a panic code instead of an opaque revert

[leonardoalt]

+1 It would be nice to have ABI decoding have a panic code instead of an opaque revert

I'm not sure that's what OP is suggesting?

[fvictorio]

To clarify, because maybe the snippet is confusing. This produces a panic code if x == 2:

    MyEnum myEnum = MyEnum(x);

This doesn't if the arg is 2:

  function f(MyEnum x) public {
  }

This issue is about this second snippet producing a panic code.

I believe this would be super useful to have, but I can close if the lack of panic here is by design and won't be changed (but it would be good to have confirmation that that's the case).

[0xalpharush]

@leonardoalt Since not returning 0x21 is by design for the OP's case, creating a new panic code for ABI decoding would be useful IMO. For instance, it would allow tools to surface meaningful error messages and could be used as feedback to generate proper inputs for fuzzers (see https://github.com/foundry-rs/foundry/issues/871).

[leonardoalt]

Sorry my bad, to me the initial title/comment sounded like it didn't revert at all, please ignore what I said before. I agree that more fine grained ABI errors are useful.

[ekpyron]

I'd say this also falls under the category of proper errors for ABI decoding reverts which we're starting to subsume under https://github.com/ethereum/solidity/issues/11664

[cameel]

I don't think it falls under #11664. That issue is about tweaking cases that already revert (i.e. replacing the text message with a uint code). Not about adding new reverts for cases that do not revert already. At least, if I was implementing #11664, it would not occur to me that I should add new reverts just based on its title and description. It does not specify which reverts are missing at all.

[ekpyron]

The issue here is not about adding new reverts, but about one case reverting with a plain revert(0,0) and the other reverting with a panic revert. Or in other words: I think you misread the issue just like Leo above :-).

[ekpyron]

As such, I think you'd agree that it does fall under #11664. If you still disagree, I may have been the one misreading this, then reopen and let me know :-).

[cameel]

Ahhh, you're right :) I fell into the same trap. I took "it does not happen" to mean that it does not revert but I now see that it meant that it reverts but not with a panic.

Ok then, this does fall squarely under #11664.