Polygon Deployment and Peer Verification

There is a rollback plan that aims to revert the proxies to the old implementation in case of a bug. This rollback plan uses pre-generated calldata. While the pre-generated calldata for the ERC20Predicate uses the updateImplementation method to schedule and execute the rollback, the calldata for the RootChainManager uses the updateAndCall method with additional data to perform a grantRole operation after the rollback. However, the grantRole function cannot be executed due to access control restrictions, and as a result, the current pre-generated calldata fails to deliver the rollback action in case a bug occurs.

The pre-generated rollback calldata for RootChainManager should be updated to call only the updateImplementation method.

The current preparation scripts optionally deploy new implementation contracts for RootChainManager, ERC20Predicate, and UChildUSDT. To deploy these contracts, a PRIVATE_KEY must be specified in the .env file or via inline environment variable assignment. This approach risks exposing and compromising private keys.

Use a hardware wallet or encrypted keystore file.

The updateTokenMigrationStatus will disable deposits and/or exits for a specific root token. Although the UpdateTokenMigrationStatus script validates non zero lastExitBlockNumber when disabling exits, this is not the case for the on chain validation.

Consider validating lastExitBlockNumber > 0 in updateTokenMigrationStatus function.

The USDT0 playbook includes rollback provisions intended for emergency scenarios. However, as per what documented, the MIGRATION_MANAGER_ROLE access is revoked from the zero address (0x0) rather than the actual address to which permissions were granted. This oversight leaves the originally authorized address with unintended and unclear permissions.

Consider revoking permissions from the address that was originally granted MIGRATION_MANAGER_ROLE access.

The preparation and migration steps involve using different foundry scripts that heavily rely on a JSON file. Although the inputs.json.example is not the final JSON that will be used during migration, it already contains wrong addresses.

Due to the fact that scripts will heavily rely on this JSON file, extra care should be taken to verify that each value is correct.

According to the upgrade README, both the RootChainManager and ERC20Predicate implementation upgrades will be performed using the same UpdateImplementation.s.sol script. The script can be run either by providing an input string or without any input. In the former case, it reads the necessary data from the provided input string; in the latter, it reads data from a previously prepared inputs.json file in the scripts/forge folder. Since the inputs.json file can only contain one upgradeImplementation key, and both upgrades use the same script, at least one of the upgrades must be performed using an input string, while the other can be read from the inputs.json file. The same case applies for UpdateTokenMigrationStatus script as well.

Consider using separate scripts for each upgrade so that all inputs can be prepared beforehand in the inputs.json file. Alternatively, modify _readInputs to read different keys from inputs.json based on whether the upgrade is for RootChainManager or ERC20Predicate.

The current playbook describes the migration steps for USDT0. These include generating calldata for creating multisig transactions, reading from a JSON file, copy and pasting results from console, testing bridge deposits and exits. The amount of manual steps may lead to human errors, due to the amount of scripts to run, pasting the incorrect value in the JSON file or into the multisig UI for building the transaction.

Consider reducing the manual inputs as much as possible. Additionally, reduce the amount of scripts to run and/or use the forge script to write the output into the JSON file. If possible, create the multisig transaction directly in the script, using the generated calldata. This avoids any UI phishing as you will be interacting with the smart contract directly.

The migrateBridgeFunds will transfer out USDT from the ERC20Predicate according to the collected USDT supply value on Polygon during the upgrade. However, these funds will be sent to a Multisig address according to the playbook, instead of the the USDT OAdapter. Additionally, the playbook does not contain any extra steps regarding a subsequent deposit of USDT to another contract. This will create an insolvency case, as there will be more USDT0 circulating in LayerZero chains than the amount of USDT available in the OAdapter to be redeemed.

After migrating USDT out of the ERC20Predicate, transfer them to the OAdapter contract, or set it as receiver in the encoded transfer call

Step 8 of the playbook README mentions that exits should be blocked from the upgrade block: "New plan is to just run the upgrade in block x, block exits from block x on (so no tx from x is valid exit) and manually check for any valid exits before the upgrade". However, the code allows exits in the upgrade block and considers them valid Reference.

        if (migrationStatus[rootToken].isExitDisabled &&
            blockNumber > migrationStatus[rootToken].lastExitBlockNumber) { // @note: if the lastExitBlockNumber is the same as the current block number, exits are still allowed
            revert("RootChainManager: EXIT_DISABLED");
        }

Update the code accordingly if the README is correct and the intention is to disallow exits in the upgrade block. However, note that this change would cause exits within the same block to fail and require manual intervention. Otherwise, update the README to reflect the current behavior.

During funds migration flow, neither RootChainManager.migrateBridgeFunds or ERC20Predicate.migrateTokens emit an event. This is a major state change that should emit relevant data, like it's currently done in the updateTokenMigrationStatus function. Offchain services will likely need to inspect the migration transaction in order to read the USDT token receiver and amount withdrawn.

Consider emitting an event either in RootChainManager or ERC20Predicate, with the token receiver and amount.

The MIGRATION_MANAGER_ROLE can block deposit/exits and later migrateBridgeFunds to withdraw tokens from ERC20Predicate. Blocking deposits/exits does not have any validation, and migration is only allowed if both deposits and exits are blocked. This means that manager may:

• call migrateBridgeFunds multiple times
• block deposits and exits to later migrate tokens, but re enable deposits or exits in the future again. This might be unexpected, as migration should be a one time action.

Consider setting a state mapping that tracks migrated tokens, preventing manager to re-migrate or re-enable deposits/exits after migration. If this is the expected behavior, document it in the RootChainManager contract.