Missing zero address check when creating a pair

[hats-bug-reporter[bot]]

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x691ff5e6ac9590db4cd9f06e63ffad1766fc1765d97a93ac996aaa8540671510 Severity: medium

Description: Description\

According to the UniswapV2 specification, when calling createPair() and creating pair contract, zero address check for the token0 is performed. This means that one of the tokens that's involved in the pair creation can be the zero address. The implementation of this check is needed to make sure that only meaningful and valid token pairs are created.

Attack Scenario\

Take a look at the UniswapV2 createPair():

https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Factory.sol#L23-38

function createPair(address tokenA, address tokenB) external returns (address pair) {
        require(tokenA != tokenB, 'UniswapV2: IDENTICAL_ADDRESSES');
        (address token0, address token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
        require(token0 != address(0), 'UniswapV2: ZERO_ADDRESS');
        require(getPair[token0][token1] == address(0), 'UniswapV2: PAIR_EXISTS'); // single check is sufficient
        bytes memory bytecode = type(UniswapV2Pair).creationCode;
        bytes32 salt = keccak256(abi.encodePacked(token0, token1));
        assembly {
            pair := create2(0, add(bytecode, 32), mload(bytecode), salt)
        }
        IUniswapV2Pair(pair).initialize(token0, token1);
        getPair[token0][token1] = pair;
        getPair[token1][token0] = pair; // populate mapping in the reverse direction
        allPairs.push(pair);
        emit PairCreated(token0, token1, pair, allPairs.length);
    }

In the amm contract, the check is missing:

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/main/amm/contracts/factory/lib.rs#L111-146

Recommendation

Add zero address check for token0.

[deuszx]

Duplicate of https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/issues/33

[rodiontr]

Duplicate of #33

What about this?

fn zero_address() -> AccountId { [0u8; 32].into() } And compare it to another address, see an example here:

ensure!(to != zero_address(), Error::ZeroAddressTransfer);

[deuszx]

"zero address" on EVM is a thing, it's not on Substrate, so they're handled differently. Of course you can create the "all zeros" address but it doesn't make sense. On Ethereum sending tokens to zero-address makes them unretrievable, that's why it's used for burning. On Substrate it cannot due to reasons mentioned in the linked comment.

[rodiontr]

"zero address" on EVM is a thing, it's not on Substrate, so they're handled differently. Of course you can create the "all zeros" address but it doesn't make sense. On Ethereum sending tokens to zero-address makes them unretrievable, that's why it's used for burning. On Substrate it cannot due to reasons mentioned in the linked comment.

if ink is used for something that's only build on Substrate, in my example, there is erc1155 implementation from the ink contracts themselves

[rodiontr]

https://github.com/paritytech/ink/blob/master/integration-tests/erc1155/lib.rs#L454

 ensure!(to != zero_address(), Error::ZeroAddressTransfer);

[deuszx]

paritytech/ink@master/integration-tests/erc1155/lib.rs#L454

 ensure!(to != zero_address(), Error::ZeroAddressTransfer);

The example is wrong, not the code here.

[rodiontr]

paritytech/ink@master/integration-tests/erc1155/lib.rs#L454

 ensure!(to != zero_address(), Error::ZeroAddressTransfer);

The example is wrong, not the code here.

sorry for such long conversations what do you mean it's wrong?

[deuszx]

I've already explained in the thread above and in the comment linked. I'd advise to first understand why this particular address is used on EVM and then reading about it in context of Substrate.

Also relevant https://substrate.stackexchange.com/questions/5929/checknonzerosender/5930#5930

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

We hope you participate in the future audits of ink!.