Feat/l2deployment #24
Conversation
ZeroEkkusu
left a comment
ZeroEkkusu
left a comment
There was a problem hiding this comment.
- Some things need to be changed. Please see the suggestions.
- Native Converters are not configured in Migration Manager on Layer X.
v0.5.0is already released. We will need to make a new release after the deployment. Please merge #25 for proper versioning.
|
|
||
| string memory vbETHSlug = string(abi.encodePacked('["', vm.toString(block.chainid), '"]', '.["vbETH"]')); | ||
|
|
||
| address vbWETH = input.readAddress(string.concat(vbETHSlug, ".customToken")); |
There was a problem hiding this comment.
Recommended to Use CustomToken and underlying token. Using vbETH for W-vbETH and WETH for W-WETH is misleading.
|
|
||
| for (uint256 i = 0; i < vbTokens.length; i++) { | ||
| address nativeConverter = | ||
| input.readAddress(string.concat(migrationManagerSlug, ".", vbTokens[i], "NativeConverter")); |
There was a problem hiding this comment.
Beware that all input.json "vbTokens[i].NativeConverters" are still fake (all of them are using address 0x000059dE96F9C28e3a343b831cbDC2B93c8C4855) , so we MUST update them once we have each of the NativeConverters deployed. Count with us to review before executing.
| "vbUSDSNativeConverter": "0x000059dE96F9C28e3a343b831cbDC2B93c8C4855", | ||
| "vbUSDTNativeConverter": "0x000059dE96F9C28e3a343b831cbDC2B93c8C4855", | ||
| "vbUSDCNativeConverter": "0x000059dE96F9C28e3a343b831cbDC2B93c8C4855", | ||
| "vbWBTCNativeConverter": "0x000059dE96F9C28e3a343b831cbDC2B93c8C4855", | ||
| "vbETHNativeConverter": "0x000059dE96F9C28e3a343b831cbDC2B93c8C4855", |
There was a problem hiding this comment.
Please update them before executing the RegisterNativeConverters.s.sol script
script/input.json
Outdated
| "customToken": "0xEE7D8BCFb72bC1880D0Cf19822eB0A2e6577aB62", | ||
| "underlyingToken": "0x815955d051C6262C16c720b19D735426254Bec5B", | ||
| "owner": "0x2De242e27386e224E5fbF110EA8406d5B70740ec", | ||
| "name": "Wrapped Ether", |
There was a problem hiding this comment.
Dangerous breaking change, be careful.
Metadata changes are usually discouraged for every sovereign token. This is due to the fact that if it is sent to a non-upgradeable bridge version in a different network this getTokenMetadata(token) call at https://github.com/agglayer/agglayer-contracts/blob/v10.1.0-rc.5/contracts/v2/PolygonZkEVMBridgeV2.sol#L397 will produce a different token address in destination network as unexpected.
This seems safe for an already deployed "upgradeable token" deployed on Katana coming from a bridge from L1 (mainnet) as it will take tokenInfoHash.originToken but beware that if sent to a 3rd network (with non upgradeable tokens - 99% of them) then it will have side effects.
To avoid address change it is encouraged to use the same metadata.
Here the issue documented https://github.com/0xPolygonHermez/internal-audit/issues/160
There was a problem hiding this comment.
Checking with the Product Team if the naming is crucial. If yes, we can override name, symbol, and decimals functions to report different values to LxLy Bridge.
There was a problem hiding this comment.
Let me put an example:
- User sends L1 vbTOK to L2 Katana (gets an upgradeable token 0xA)
- Katana W-vbTOK token 0xA's owner changes its metadata to a different name / symbol (i.WETH).
- User sends Katana 0xA W-vbTOK (renamed as WETH) to network LY with a non-uogradeable version (gets a different address 0xB
- the problem is not 0xA != 0xB because those networks use different bridge type (upgradeable wrapped tokens vs non upgradeable)
- the problem is that non-upgradeable networks will have different addresses for the same asset depending on whether or not the first deposit came from Katana (with a renamed metadata) or for example L1 with the original naming.
- so the problem is that if a user sends vbTOK from L1 to LY+1, that W-vbTOK will get an address of 0xC which is different from != 0xB
There was a problem hiding this comment.
The solution I proposed fixes the issue because LxLy Bridge will see the original metadata, while everyone else will see the new metadata. So, the same bridge-wrapped token will be used no matter if you bridge from Ethereum or Katana.
There was a problem hiding this comment.
@simonDos Per the internal discussions, revert the naming to Vault Bridge ETH / vbETH.
This PR prepares VaultBridge to be deploy on Katana.
An important fix has been added in
src/CustomToken.solto allow bridging to address(0).Other than that, the contracts itself are untouched.
Version is still
0.5.0as this is only the 2nd part of the deployment, now on Katana.Deployment scripts are:
script/DeployLayerY.s.solscript/DeployLayerY_WETH.s.solThe
script/input.jsonat747474contains all important deployment configurations, settings and addresses.It needs to be thoroughly reviewed.
The scripts print payloads for upgrading the custom tokens (bridge-wrapped vbTokens).
These will need to be executed (in bulk) by the Polygon Engineering Multisig on Katana:
0x4e981bAe8E3cd06Ca911ffFE5504B2653ac1C38a