Verification of address is different from intent.

[cdor1]

Describe the bug
It seems ehters.js supports multiple types of addresses in ethereum.
But, contrary to the intended implementation, ethers.js does not support the addresses without '0x' or addresses starts with '0X' that web3.utils.isAddress() supports.

I think it can be a security bug when developers fully trust the results of web3.utils.isAddress() function and sending transactions with those addresses. It may cause an exception and remote DoS occurs.

Sample Contract to Run the PoC

pragma solidity >=0.8.0;
contract sample {
    function somefunc(address account) public pure returns(address){
        return account;
    }
}

Reproduction steps 1
When I use the addresses without '0x', it causes "invalid BigNumber string" error.

PoC

import Web3 from "web3";
import { Contract } from "web3-eth-contract";

(async () => {
    const web3 = new Web3([...]);

    const contract = new web3.eth.Contract([abi], [contract address]);
    const address = "c1912fee45d61c87cc5ea59dae31190fffff232d";
    console.log(await contract.methods.somefunc(address).send({from: [sender address]}));
})();

Result

(node:71332) UnhandledPromiseRejectionWarning: Error: invalid BigNumber string (argument=“value”, value=“c1912fee45d61c87cc5ea59dae31190fffff232f”, code=INVALID_ARGUMENT, version=bignumber/5.4.1)

Root Cause
In getAddress() function, if the address passed without '0x', it adds '0x' and checks the checksum.

//https://github.com/ethers-io/ethers.js/blob/ba404ffb0b77f95f37172a5e52da0f7949a7f406/packages/address/lib.esm/index.js#L67-L75
export function getAddress(address) {
    let result = null;
    if (typeof (address) !== "string") {
        logger.throwArgumentError("invalid address", "address", address);
    }
    if (address.match(/^(0x)?[0-9a-fA-F]{40}$/)) {
        // Missing the 0x prefix
        if (address.substring(0, 2) !== "0x") {
            address = "0x" + address;

But, In the AddressCoder.encode() function that calls getAddress(), it does not append '0x' to the address and call the writer.writeValue()

// https://github.com/ethers-io/ethers.js/blob/master/packages/abi/src.ts/coders/address.ts#L18-L24
    encode(writer: Writer, value: string): number {
        try {
            getAddress(value);
        } catch (error) {
            this._throwError(error.message, value);
        }
        return writer.writeValue(value);
    }

In the writer.writeValue() function, it parses the address as BigNumber. but, the passed address does not have '0x', and the address can have [a-z0-9A-Z]. So, the below regexp not matches and throw the "invalid BigNumber string" error.

// https://github.com/ethers-io/ethers.js/blob/master/packages/bignumber/src.ts/bignumber.ts#L228-L241
    static from(value: any): BigNumber {
        if (value instanceof BigNumber) { return value; }

        if (typeof(value) === "string") {
            if (value.match(/^-?0x[0-9a-f]+$/i)) {
                return new BigNumber(_constructorGuard, toHex(value));
            }

            if (value.match(/^-?[0-9]+$/)) {
                return new BigNumber(_constructorGuard, toHex(new BN(value)));
            }

            return logger.throwArgumentError("invalid BigNumber string", "value", value);
        }

Reproduction steps 2
When I use the addresses that start with '0X', it causes "invalid address" error

PoC

import Web3 from "web3";
import { Contract } from "web3-eth-contract";

(async () => {
    const web3 = new Web3([...]);

    const contract = new web3.eth.Contract([abi], [contract address]);
    const address = "0XC1912FEE45D61C87CC5EA59DAE31190FFFFF232D";
    console.log(await contract.methods.somefunc(address).send({from: [sender address]}));
})();

Result

(node:59056) UnhandledPromiseRejectionWarning: Error: invalid address (argument="address", value="0XC1912FEE45D61C87CC5EA59DAE31190FFFFF232D", code=INVALID_ARGUMENT, version=address/5.4.0) (argument="account", value="0XC1912FEE45D61C87CC5EA59DAE31190FFFFF232D", code=INVALID_ARGUMENT, version=abi/5.0.7)

Root Cause
In getAddress() function, the regexp does not allows the address with '0X'.

// https://github.com/ethers-io/ethers.js/blob/master/packages/address/lib.esm/index.js#L65-L97
export function getAddress(address) {
    let result = null;
    if (typeof (address) !== "string") {
        logger.throwArgumentError("invalid address", "address", address);
    }
    if (address.match(/^(0x)?[0-9a-fA-F]{40}$/)) {
      // ...
    }
    else if (address.match(/^XE[0-9]{2}[0-9A-Za-z]{30,31}$/)) {
        // ...
        }
        result = getChecksumAddress("0x" + result);
    }
    else {
        logger.throwArgumentError("invalid address", "address", address);
    }
    return result;
}

Environment:
Node

[ricmoo]

I won't be extending the allowed addresses to have a 0X prefix, but the prefix 0x should certainly be allowed. I've added a test case too for this case, and have fixed it locally. I will be cutting a release later to day which will include these changes.

I wasn't quite sure what you meant by the address can have [a-z0-9A-Z], an address must still be a hexdata string. Or do you mean when passes as an ICAP address?

Thanks!

[cdor1]

I meant the addresses that have checksum, but no '0x' prefix.
Sorry, I missed the /i flag in regexp. There is no problem.

// https://github.com/ethers-io/ethers.js/blob/master/packages/bignumber/src.ts/bignumber.ts#L228-L241
    static from(value: any): BigNumber {
        if (value instanceof BigNumber) { return value; }

        if (typeof(value) === "string") {
            if (value.match(/^-?0x[0-9a-f]+$/i)) { // my mistake
                return new BigNumber(_constructorGuard, toHex(value));
            }

My personal question is, is there any reason not to allow '0X' prefixed addresses? Or, I wonder why web3.utils.isAddress allowed it.

[ricmoo]

Mainly that making that change could have other consequences I'm not thinking of right now, as those hexstrings and hexdatastrings are used in many places, across other RegExps, string matching, string comparison, etc.

But I also find the 0x prefix to be something that is explicit and demonstrates intention. I have never seen 0X outside the context of someone simply using .toUpperCase() or similarly blinding manipulating strings. I've never typed it intending to mean an address. :)

Ethers has never strived to be compatible with Web3.js; a lot of it is, by virtue that they both aim to provide an Ethereum-friendly experience, but a lot of decisions for Web3.js were made very early, under tight deadlines, and they have had to live with a lot of legacy as a result. One of the biggest things I've strived for with ethers is never guessing and only coercing when the intent is obvious.

I expect people using ethers to use the getAddress() or isAddress() provided by ethers; mixing and matching ethers and Web3.js will likely have other problems. I don't recommend it. :)

I'm more that open to have my mind changed though, if there are good reasons to support it (other than "web3.js does it" ;)).

[cdor1]

Thanks for comment.
Since many parts of web3 trust and use ethers codes, I have explained them compared to web3.
I didn't mean to force compatibility with web3.js. Sorry.

[cdor1]

The bug I delivered seems to have been fixed, can I close the issue?
Thanks for the quick and stable fix.

[ricmoo]

I can close it shortly. The newest version hasn't been published yet (the CI is having a few hiccups), but once it's up, I'll take care of it. :)

No worries at all, I just try to remind people from time to time (it comes up a lot), that ethers is meant to be its own library, not just an alternative to Web3.js. The Web3.js definitely satisfy a need and has a lot of legacy applications they have to keep the lights on for. :)

Thanks! :)

[ricmoo]

(thanks for finding the above issue, btw; the address coder not properly converting ICAP addresses and addresses without a 0x prefix :))

[cdor1]

OK, Thank you for creating and managing a good library.😃

[ricmoo]

This has been updated in 5.4.6.

Let me know if you have any further issues.

Thanks! :)

[ricmoo]

Closing now. If you have any issues please feel free to re-open or start a new issue.

Thanks! :)