readInt automatic base detection is a footgun

[dcposch]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (75836a7 2023-09-09T00:31:54.458033000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

JSON reading functions readInt/readUint/readIntArray etc automatically detect the base of the string.

This leads to unpleasant surprises... in our case we had a P256 verifier working on all but four of ~300 Wycheproof test vectors, and the last four were due to readUint switching to decimal.

Minimal reproducible example

import {stdJson} from "forge-std/StdJson.sol";
import {Test, console2} from "forge-std/Test.sol";

using stdJson for string;

contract ReadIntTest is Test {
    function testReadInt() public {
        string memory json = '["ffffffff","00000010"]';
        int256[] memory ints = json.readIntArray("");
        console2.log(ints[0]);
        console2.log(ints[1]);
    }
}

Proposed fix

• Parse decimal only
• (If there's demand for it, you could parse hex when there is an 0x prefix)

Less is more. If people pass a bare hex string to readInt, better to error right away (then they can easily uint256(json.readBytes32(...))) than sneakily, later, once some input lacks letters.

[mattsse]

This leads to unpleasant surprises... in our case we had a P256 verifier working on all but four of ~300 Wycheproof test vectors, and the last four were due to readUint switching to decimal.

sorry, can you elaborate on this, not entirely sure what this means.

what's the issue?

EDIT:
ah so parsing is correct, but support for various formats is unexpected? what would have been the expected behaviour for a hex or bin here? an error?

[dcposch]

Yes, exactly. In all modern standard libraries I know (Rust, Go, etc) parseInt("10") or similar returns 10, parseInt("ff") throws an error. Runtime heuristics are bad especially in this context (smart contract testing).

[mds1]

Makes sense. Is an acceptable solution to update the parseJsonInt/parseJsonUint family of cheats (what readInt etc. uses under the hood) such that:

• If the string only contains decimal values, parse as decimal
• If the string starts with 0x and only contains hex values, parse as hex
• Otherwise, throw an error

There are people who prefer hex strings, especially for large ints, so I am hesitant to only support decimal. But requiring the 0x prefix to indicate hex seems like a good tradeoff

[dcposch]

@mds1 yes, sounds perfect. key is it's deterministic: string either starts with 0x (and is parsed as hex) or not (and is parsed as decimal), no guessing.

[mds1]

cc @Evalir, this should be an easy one to fix to remove the footgun

[ruvaag]

@mds1 @Evalir I can take this up, but might need some help.

Couple of things I came across:

• This behaviour appears to be bubble from the parity-common/uint U256 implementation
• Under the hoodparseJsonInt and parseJsonUint cheats use the same parse function also used by parseInt and parseUint (ext::parse_json calls parse::parse) which couples their behaviour.

Given this, one way way to proceed could be to explicitly check if a non-prefix string contains only numeric characters before parsing it as a uint / int across all kinds of cheats? Curious if there's a better way or if I'm missing something here.

[Evalir]

happy to assign it to you @ruvaag !

indeed, this is bubbling up from the parity types crate. What you propose is what I'd do—only accept decimal characters before parsing as uint/int. Eventually we'll switch to alloy types and this will be a tad easier to do.