Skip to content

Fix BooleanCoder.decode to error on non-zero padding and invalid boolean values #5001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KindKillerwhale
Copy link

During fuzzing, I discovered that bug. Ethers.js currently decodes a bool by treating any non-zero 32-byte word as true, but go-ethereum’s readBool requires the first 31 bytes to be zero and the final byte to be exactly 0x00 or 0x01, throwing an error otherwise. This mismatch can lead to silent misinterpretation of invalid boolean data, cross-client inconsistencies, and hidden bugs, so we must enforce strict padding and value checks in BooleanCoder.decode.

Go Bool type - Go-Ethereum

Go-Ethereum

// go-ethereum/accounts/abi/unpack.go
func readBool(word []byte) (bool, error) {
    for _, b := range word[:31] {
        if b != 0 {
            return false, errBadBool  // Non-zero padding byte -> error
        }
    }
    switch word[31] {
    case 0: return false, nil
    case 1: return true, nil
    default: return false, errBadBool  // Invalid boolean value -> error
    }
}

In the Ethereum ABI, the bool type is encoded as a 32-byte word, similar to uint256, and the value is restricted to 0 or 1. The decoder in geth enforces this strictly. The upper 31 bytes must all be 0, and the last byte must be either 0x00 or 0x01.

If any of the first 31 bytes is non-zero, or the last byte is not 0 or 1, Go will return an error called errBadBool.

💡

Ex) 0x000...002 → If the last byte is 0x02, it returns errBadBool

JS - ethers.js

// ethers.js/src.ts/abi/coders

  decode(reader: Reader): any {
      return !!reader.readValue();
  }
  • readValue() directly returns a BigInt.
  • !!BigInt(0)false, !!BigInt(nonzero)true

This treats any non-zero 32-byte word as true.
It ignores the standard ABI requirement that the first 31 bytes must be zero and the last byte must be exactly 0x00 or 0x01.

Solution

  decode(reader: Reader): any {
      const bytes = reader.readBytes(32, false);

      for (let i = 0; i < 31; i++) {
          assert(
              bytes[i] === 0,
              `Boolean padding error: byte[${i}]=0x${bytes[i].toString(16)}`,
              "INVALID_ARGUMENT"
          );
      }

      const v = bytes[31];
      assert(
          v === 0 || v === 1,
          `Boolean value error: expected 0 or 1 but got 0x${v.toString(16)}`,
          "INVALID_ARGUMENT"
      );
      return v === 1;
  }

Changes

  • Update packages/abi/src/coders/boolean.ts

    • Replace !!reader.readValue() with explicit padding + value checks.
  • Add unit tests for:

    • Non-zero padding → throws “Boolean padding error”
    • Invalid last-byte (e.g. 0x02) → throws “Boolean value error”
    • Valid cases (0x00false, 0x01true)

This ensures ethers.js boolean decoding is fully compliant with the Ethereum ABI spec and consistent with Go-Ethereum’s implementation.

@ricmoo
Copy link
Member

ricmoo commented May 30, 2025

I believe this used to be done, but breaks other things. It’s been a long time since I wrote that.

But accepting input that other packages won’t accept isn’t really a bug I don’t think? The more important question is what happens in Solidiity; will solidity throw an exception if 2 is passed in as a bool? I think I’ve done this test before and Solidity keeps-on-truckin’ which is far more important (compared to a geth API) to align with.

I might also considering contacting Geth to see what they think, as they likely want to be solidity compatible. Depending where they use this feature.

Also currently it means ethers can be used to normalize (as its output will always be standards compliant) by reading in a value and then writing it back out.

At the very least, it would need to be made as an opt-in feature (similar to disabling strict mode) as to not break existing code.

Make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants