Skip to content
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.

Sign up for GitHub

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

SMR-1782 ETH Child Bridge #9

Merged
merged 16 commits into from
Oct 25, 2023
Merged

SMR-1782 ETH Child Bridge #9

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
82d032c
initialising with child eth token implemented and tested
proletesseract Oct 19, 2023
ee559c7
Update RootERC20Bridge.t.sol
proletesseract Oct 19, 2023
877195d
Merge branch 'sprint-1-poc' into SMR-1782-eth-bridging-child
proletesseract Oct 19, 2023
c4c59a4
first draft
proletesseract Oct 20, 2023
22a0f27
Update InitializeChildContracts.s.sol
proletesseract Oct 20, 2023
932ad87
Update ChildERC20Bridge.sol
proletesseract Oct 20, 2023
6a06077
Update ChildERC20Bridge.sol
proletesseract Oct 20, 2023
fc2eaae
Update .gitignore
proletesseract Oct 24, 2023
e4de38e
remove ETH_TOKEN address from child initialise
proletesseract Oct 24, 2023
1d5fcac
Merge branch 'sprint-1-poc' into SMR-1782-eth-bridging-child
proletesseract Oct 24, 2023
c597a9f
formatting
proletesseract Oct 24, 2023
eb4134e
integration tests for IMX and Native events
proletesseract Oct 24, 2023
108abc4
removed childETH from root and added integration tests
proletesseract Oct 24, 2023
abc5c4f
formatting
proletesseract Oct 24, 2023
59e4a8c
Remove need for child eth token env var in deployment scripts
Benjimmutable Oct 24, 2023
56737b4
Predict the child ETH address properly
Benjimmutable Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion script/DeployChildContracts.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ contract DeployChildContracts is Script {
function run() public {
uint256 deployerPrivateKey = vm.envUint("CHILD_PRIVATE_KEY");
address childGateway = vm.envAddress("CHILD_GATEWAY_ADDRESS");
address childGasService = vm.envAddress("CHILD_GAS_SERVICE_ADDRESS"); // Not yet used.
//address childGasService = vm.envAddress("CHILD_GAS_SERVICE_ADDRESS"); // Not yet used.
string memory childRpcUrl = vm.envString("CHILD_RPC_URL");

vm.createSelectFork(childRpcUrl);
Expand Down
4 changes: 3 additions & 1 deletion script/InitializeChildContracts.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ contract InitializeChildContracts is Script {
string memory childRpcUrl = vm.envString("CHILD_RPC_URL");
string memory rootChainName = vm.envString("ROOT_CHAIN_NAME");
address rootIMXToken = vm.envAddress("ROOT_IMX_ADDRESS");
address rootETHToken = vm.envAddress("ROOT_ETH_ADDRESS");

/**
* INITIALIZE CHILD CONTRACTS
Expand All @@ -31,7 +32,8 @@ contract InitializeChildContracts is Script {
Strings.toHexString(rootERC20BridgeAdaptor),
childTokenTemplate,
rootChainName,
rootIMXToken
rootIMXToken,
rootETHToken
);

childAxelarBridgeAdaptor.setRootBridgeAdaptor();
Expand Down
58 changes: 42 additions & 16 deletions src/child/ChildERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,38 +33,49 @@ contract ChildERC20Bridge is
{
using SafeERC20 for IERC20Metadata;

/// @dev leave this as the first param for the integration tests
mapping(address => address) public rootTokenToChildToken;

bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN");
bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT");
address public constant NATIVE_ETH = address(0xeee);

IChildERC20BridgeAdaptor public bridgeAdaptor;

/// @dev The address that will be sending messages to, and receiving messages from, the child chain.
string public rootERC20BridgeAdaptor;
/// @dev The address of the token template that will be cloned to create tokens.
address public childTokenTemplate;
/// @dev The name of the chain that this bridge is connected to.
string public rootChain;
mapping(address => address) public rootTokenToChildToken;

bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN");
bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT");
address public imxToken;
/// @dev The address of the IMX ERC20 token on L1.
address public rootIMXToken;
/// @dev The address of the ETH ERC20 token on L2.
address public childETHToken;

/**
* @notice Initilization function for RootERC20Bridge.
* @param newBridgeAdaptor Address of StateSender to send deposit information to.
* @param newRootERC20BridgeAdaptor Stringified address of root ERC20 bridge adaptor to communicate with.
* @param newChildTokenTemplate Address of child token template to clone.
* @param newRootChain A stringified representation of the chain that this bridge is connected to. Used for validation.
* @param newIMXToken Address of ECR20 IMX on the root chain.
* @param newRootIMXToken Address of ECR20 IMX on the root chain.
* @param newRootETHToken Address used to denote ETH on the root chain.
* @dev Can only be called once.
*/
function initialize(
address newBridgeAdaptor,
string memory newRootERC20BridgeAdaptor,
address newChildTokenTemplate,
string memory newRootChain,
address newIMXToken
) public initializer {
address newRootIMXToken,
address newRootETHToken)
Copy link
Contributor

@Benjimmutable Benjimmutable Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@proletesseract proletesseract Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it from the initialiser and updated tests.

public initializer
{
if (newBridgeAdaptor == address(0)
|| newChildTokenTemplate == address(0)
|| newIMXToken == address(0)) {
|| newRootIMXToken == address(0)
|| newRootETHToken == address(0)) {
revert ZeroAddress();
}

Expand All @@ -80,7 +91,12 @@ contract ChildERC20Bridge is
childTokenTemplate = newChildTokenTemplate;
bridgeAdaptor = IChildERC20BridgeAdaptor(newBridgeAdaptor);
rootChain = newRootChain;
imxToken = newIMXToken;
rootIMXToken = newRootIMXToken;

IChildERC20 clonedETHToken =
IChildERC20(Clones.cloneDeterministic(childTokenTemplate, keccak256(abi.encodePacked(newRootETHToken))));
clonedETHToken.initialize(newRootETHToken, "Ethereum", "ETH", 18);
childETHToken = address(clonedETHToken);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@proletesseract proletesseract Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -122,7 +138,7 @@ contract ChildERC20Bridge is
revert ZeroAddress();
}

if (address(rootToken) == imxToken) {
if (address(rootToken) == rootIMXToken) {
revert CantMapIMX();
}

Expand All @@ -149,19 +165,29 @@ contract ChildERC20Bridge is

address childToken;

if (address(rootToken) != imxToken) {
childToken = rootTokenToChildToken[address(rootToken)];
if (childToken == address(0)) {
revert NotMapped();
if (address(rootToken) != rootIMXToken) {
if (address(rootToken) == NATIVE_ETH) {
childToken = childETHToken;
} else {
childToken = rootTokenToChildToken[address(rootToken)];
if (childToken == address(0)) {
revert NotMapped();
}
}

if (address(childToken).code.length == 0) {
revert EmptyTokenContract();
}

if (!IChildERC20(childToken).mint(receiver, amount)) {
revert MintFailed();
}
emit ERC20Deposit(address(rootToken), childToken, sender, receiver, amount);

if (address(rootToken) == NATIVE_ETH) {
emit NativeEthDeposit(address(rootToken), childToken, sender, receiver, amount);
} else {
emit ERC20Deposit(address(rootToken), childToken, sender, receiver, amount);
}
} else {
Address.sendValue(payable(receiver), amount);
emit IMXDeposit(address(rootToken), sender, receiver, amount);
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/child/IChildERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ interface IChildERC20BridgeEvents {
address indexed receiver,
uint256 amount
);
event NativeDeposit(
event NativeEthDeposit(
address indexed rootToken,
address indexed childToken,
address depositor,
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/root/IRootERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ interface IRootERC20BridgeEvents {
address indexed receiver,
uint256 amount
);
event NativeDeposit(
event NativeEthDeposit(
address indexed rootToken,
address indexed childToken,
address depositor,
Expand Down
15 changes: 8 additions & 7 deletions src/root/RootERC20Bridge.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,20 @@ contract RootERC20Bridge is
{
using SafeERC20 for IERC20Metadata;

/// @dev leave this as the first param for the integration tests
mapping(address => address) public rootTokenToChildToken;

bytes32 public constant MAP_TOKEN_SIG = keccak256("MAP_TOKEN");
bytes32 public constant DEPOSIT_SIG = keccak256("DEPOSIT");
address public constant NATIVE_TOKEN = address(0xeee);
address public constant NATIVE_ETH = address(0xeee);

IRootERC20BridgeAdaptor public rootBridgeAdaptor;
/// @dev Used to verify source address in messages sent from child chain.
/// @dev Stringified version of address.
string public childBridgeAdaptor;
/// @dev The address that will be minting tokens on the child chain.
address public childERC20Bridge;
/// @dev The address of the token template that will be cloned to create tokens on the child chain.
address public childTokenTemplate;
mapping(address => address) public rootTokenToChildToken;
/// @dev The address of the IMX ERC20 token on L1.
address public rootIMXToken;
/// @dev The address of the ETH ERC20 token on L2.
Expand Down Expand Up @@ -110,7 +111,7 @@ contract RootERC20Bridge is

uint256 expectedBalance = address(this).balance - (msg.value - amount);

_deposit(IERC20Metadata(NATIVE_TOKEN), receiver, amount);
_deposit(IERC20Metadata(NATIVE_ETH), receiver, amount);

// invariant check to ensure that the root native balance has increased by the amount deposited
if (address(this).balance != expectedBalance) {
Expand Down Expand Up @@ -186,7 +187,7 @@ contract RootERC20Bridge is
// TODO We can call _mapToken here, but ordering in the GMP is not guaranteed.
// Therefore, we need to decide how to handle this and it may be a UI decision to wait until map token message is executed on child chain.
// Discuss this, and add this decision to the design doc.
if (address(rootToken) != NATIVE_TOKEN) {
if (address(rootToken) != NATIVE_ETH) {
if (address(rootToken) != rootIMXToken) {
childToken = rootTokenToChildToken[address(rootToken)];
if (childToken == address(0)) {
Expand All @@ -206,8 +207,8 @@ contract RootERC20Bridge is

rootBridgeAdaptor.sendMessage{value: feeAmount}(payload, msg.sender);

if (address(rootToken) == NATIVE_TOKEN) {
emit NativeDeposit(address(rootToken), childETHToken, msg.sender, receiver, amount);
if (address(rootToken) == NATIVE_ETH) {
emit NativeEthDeposit(address(rootToken), childETHToken, msg.sender, receiver, amount);
} else if (address(rootToken) == rootIMXToken) {
emit IMXDeposit(address(rootToken), msg.sender, receiver, amount);
} else {
Expand Down
9 changes: 5 additions & 4 deletions test/integration/child/ChildAxelarBridge.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import {Utils} from "../../utils.t.sol";
contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChildERC20BridgeErrors, Utils {
string public ROOT_ADAPTOR_ADDRESS = Strings.toHexString(address(1));
string public ROOT_CHAIN_NAME = "ROOT_CHAIN";
Comment on lines 18 to 20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at adding these integration tests now.

Copy link
Contributor Author

@proletesseract proletesseract Oct 24, 2023 •
edited by Benjimmutable
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

address constant IMX_TOKEN = address(9);
address constant IMX_TOKEN_ADDRESS = address(0xccc);
address constant CHILD_ETH_TOKEN = address(0xddd);

ChildERC20Bridge public childERC20Bridge;
ChildERC20 public childERC20;
Expand All @@ -35,7 +36,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
new ChildAxelarBridgeAdaptor(address(mockChildAxelarGateway), address(childERC20Bridge));

childERC20Bridge.initialize(
address(childAxelarBridgeAdaptor), ROOT_ADAPTOR_ADDRESS, address(childERC20), ROOT_CHAIN_NAME, IMX_TOKEN
address(childAxelarBridgeAdaptor), ROOT_ADAPTOR_ADDRESS, address(childERC20), ROOT_CHAIN_NAME, IMX_TOKEN_ADDRESS, CHILD_ETH_TOKEN
);
}

Expand Down Expand Up @@ -246,8 +247,8 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
bytes32 depositSig = childERC20Bridge.DEPOSIT_SIG();
address rootAddress = address(0x123);
{
// Slot is 6 because of the Ownable, Initializable contracts coming first.
uint256 rootTokenToChildTokenMappingSlot = 6;
// Slot is 2 because of the Ownable, Initializable contracts coming first.
uint256 rootTokenToChildTokenMappingSlot = 2;
address childAddress = address(444444);
bytes32 slot = getMappingStorageSlotFor(rootAddress, rootTokenToChildTokenMappingSlot);
bytes32 data = bytes32(uint256(uint160(childAddress)));
Expand Down
Loading