Native Token Bridge

[geoff-vball]

Why this should be merged

This adds a pair of contracts that can move a native token from an existing EVM chain into a new EVM chain as the new chain's native token. For instance, you could create a subnet that uses AVAX from the C-chain as its native token used to pay for transaction fees in that subnet.

See #22

How this works

See contracts/src/CrossChainApplications/NativeTokenBridge/README.md

How this was tested

e2e tests added

How is this documented

See contracts/src/CrossChainApplications/NativeTokenBridge/README.md

[michaelkaplan13]

Some initial thoughts:

• Lets go with NativeTokenSource and NativeTokenDestination contract naming for now. We can keep thinking/iterating here.
• We'll add the respective source/destination addresses as constructor arguments to each.
• Lets rename bridgeTokens to transferToDestination and transferToSource. Can also keep thinking here.
• We'll add the amount pre-minted in the genesis as a constructor argument to the NativeTokenDestination contract. Minting and releasing will be disabled until initial supply is collateralized.
• Add a NativeTokenERC20Source implementation that accounts for the decimal count of the ERC20.

[minghinmatthewlam]

mostly LGTM, left some nits and I didn't see reportTotalBurnedTxFees defined in the interface for INativeTokenDestination. Other than that, can you create new issues regarding comments that will be deferred.

[minghinmatthewlam]

same, can we use interfaces.Network here instead?

[geoff-vball]

It was interfaces.Network, but Cam suggested interfaces.LocalNetwork. I think that's the right decision because these tests won't be able to be run on a testnet, since they require their own subnet that has admin address set for the Native Minter precompile. So while we could spin up a subnet on Fuji to test these contracts, the flow would have to be different from what is here.

[minghinmatthewlam]

sounds good no strong pref here. After this gets deployed and a fuji subnet has it set up, would we be able to use that subnet as a testnet if wanted?

[geoff-vball]

We could test the functionality, yeah, but we would need to not use the same setup code. So we could split them out at that point of setup and then test.

[openzeppelin-code]

Native Token Bridge

Generated at commit: 2f1059716797391a2ad5d7efdea3d4276c130da9

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 0 0 5 23 28
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

---

For more details view the full report in OpenZeppelin Code Inspector

[michaelkaplan13]

Handful of final nits but otherwise LGTM

[michaelkaplan13]

The OZ Defender report suggests using recipient.call{value: amount, gas: XXX}("") here. I think this is probably a worthwhile change because it's possible for the fallback method of a contract to be called to execute code. We have the proper nonReentrant guard in place already to prevent against any reentrancy, so I think we can use the full gasleft() amount in the call.

This could also apply to the two other places we use .transfer() in the new contracts, and I'd say is worth updating these to be consistent, but given that they only are transferring to the burn address it shouldn't be possible for that to call code either way.

[geoff-vball]

@michaelkaplan13 What do you think of using https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6ba452dea4258afe77726293435f10baf2bed265/contracts/utils/Address.sol#L41?

Just using recipient.call{value: amount, gas: XXX}("") actually bloats to
(bool success, ) = BURNED_TX_FEES_ADDRESS.call{value: amount, gas: gasleft()}(""); require(success, "NativeTokensource: failed to burn tokens");
Because we have to check the return status of call.

[michaelkaplan13]

Didn't know that existed, but looks like exactly what we'd want 👍

[michaelkaplan13]

I'm not 100% sure, but I think .call() passes along the remaining gas if not explicitly set also.

[michaelkaplan13]

Confirmed that here.

You can also leave the gas amount, then it will forward all gas and let the called contract execute its logic and return the remainder. Safer is to provide an upper limit, just in case.

Great find with that OZ library, lets definitely use it 👍

[minghinmatthewlam]

@michaelkaplan13 can you clarify your consideration of fallback functions in terms of comparing tranfser and call? IIUC, both transfer and low level call handle fallback functions the same, and both are guarded through reentrancy guards, so the difference is the gas passed down

[michaelkaplan13]

My understanding is that transfer always uses at most 2300 gas, which creates a hard limit on the amount of gas available to any fallback function. That becomes problematic if the gas cost of operations change in network upgrades because fallback functions that may have used <=2300 gas prior may need >2300 afterwards. Using call lets them use as much gas is provided in the transaction, so they could still work even if they use more gas after future upgrades.

[geoff-vball]

Yeah, that's right. There's a helpful comment above the sendValue implementation I also linked above.

[minghinmatthewlam]

okay yup thats my understanding as well, thanks!

[michaelkaplan13]

🚢 🚀

I think it looks ready to roll! Excited to get it merged and start the follow up issues soon also.

[minghinmatthewlam]

LGTM thanks for pushing through comments and making new issues! 🚀

[businezzers]

Wow