SMR-1782 ETH Child Bridge

[proletesseract]

The child bridge side of ETH bridging is now complete.

Callouts

I've moved the rootTokenToChildToken mapping to the first param and left a comment there about not moving it so we don't have to keep changing the slots in the tests when we add more params.

I've re-organised some of the params to be in the same order across the root and child bridge contracts.

I've renamed some of the variables we were using to be a bit more clear, for example NATIVE_TOKEN -> NATIVE_ETH because we were also using it as NATIVE_TOKEN on the child side, which is a bit confusing since the native token there is IMX.

I've set the child contract up to launch the ETH ERC20 token contract on layer 2 when the bridge is initialised, so we don't need to worry about that being a manual step.

I added some tests to the initialise function but was unable to figure out how to compare the address of the deployed ETH ERC20 was the one we were predicting, so i just got it to check the address is non zero and the contract code is non zero length.

Tests should be pretty complete, most of it is already covered by other tests as the logic divergence is very minimal to other ERC20 deposits.

[ermyas]

Out of curiosity, is there a reason why we store a reference to the ETH token contract separately from other mapped tokens? I'm wondering if we could apply the same approach here, and map newRootETHToken to address(clonedETHToken), in the rootTokenToChildToken mapping.

[proletesseract]

i only really did it like this to mirror the decision to leave the L1 ERC20 IMX -> L2 Native IMX mapping out of the token mapping and handle it separately. I think largely because on the native side of the pair there is technically no token address. Even though at the moment we use a made up address to denote the native token, that's not necessarily the only way we could handle that relationship. I want to look into the other bridge contracts a bit more as a reference on how they handle native mappings. If we stick with using an address to represent the native token and we're confident that won't cause any problems, i believe we could store it in the token mapping instead of separately. Open to suggestions on this.

[proletesseract]

it looks like the Polygon bridge does use a mock address and adds it to the token mapping in the initialize function on the root bridge.

 if (nativeTokenRootAddress != address(0)) {
    rootTokenToChildToken[nativeTokenRootAddress] = 0x0000000000000000000000000000000000001010;
    emit TokenMapped(nativeTokenRootAddress, 0x0000000000000000000000000000000000001010);
}

https://github.com/0xPolygon/core-contracts/blob/main/contracts/root/RootERC20Predicate.sol

And again in the initialize function in the child bridge;

if (newNativeTokenRootAddress != address(0)) {
    rootTokenToChildToken[newNativeTokenRootAddress] = NATIVE_TOKEN_CONTRACT;
    // slither-disable-next-line reentrancy-events
    emit L2TokenMapped(newNativeTokenRootAddress, NATIVE_TOKEN_CONTRACT);
}

https://github.com/0xPolygon/core-contracts/blob/main/contracts/child/ChildERC20Predicate.sol

[proletesseract]

i think keeping the native tokens out of the mapping is the most secure. I've added checks on the mapToken functions to block the mapping of the hardcoded ETH address as the root token. This means no one can try to map an ERC20 with that reserved address (if it's even possible) and since we don't allow bridging of unmapped tokens it means we can be sure no one can deposit some scam coin on L1 and receive ERC20 ETH on L2.

[Benjimmutable]

We use newRootETHToken here in the initializer, but we have the storage constant NATIVE_ETH which is used in other parts of the contract to check the root token address and if it represents ETH. This could be confusing

[proletesseract]

i think you're right, we don't need to parse this in on initialization since it wont change between environments/networks unlike the rootIMXToken address. I will remove this from the initializer update the tests and rely on the hardcoded value.

[proletesseract]

removed it from the initialiser and updated tests.

[Benjimmutable]

I think there is some coverage that is missing in the child bridge integration tests now. Namely, making sure that if the root chain's ETH address is used, that the child ETH address is chosen, and that NativeEthDeposit is emitted (as these are different branches). They are tested in the unit test but think it could be nice adding here too

[proletesseract]

looking at adding these integration tests now.

[proletesseract]

ive added more integration tests to the root and child. also removed the need for the childETH address to be passed into the rootBridge initialize method and updated tests + deploy scripts to reflect that.

[proletesseract]

resolved merge conflicts. ran forge fmt

[Benjimmutable]

We shouldn't actually deploy the token here, we should use the prediction function with the child bridge as the deployer. Otherwise it will yield an incorrect address

[Benjimmutable]

Something like this:

            Clones.predictDeterministicAddress(childTokenTemplate, keccak256(abi.encodePacked(NATIVE_ETH)), childBridge);

See the mapTokens function for reference

[Benjimmutable]

Addressed