Transceiver invariants and ownership synchronicity can be broken by unsafe Transceiver upgrades
Transceivers are upgradeable contracts integral to the cross-chain message handling of NTT tokens. While `WormholeTransceiver` is a specific implementation of the `Transceiver` contract, NTT Managers can integrate with Transceivers of any custom implementation.\\n`Transceiver::_checkImmutables` is an internal virtual function that verifies that invariants are not violated during an upgrade. Two checks in this function are that a) the NTT Manager address remains the same and b) the underlying NTT token address remains the same.\\nHowever, the current logic allows integrators to bypass these checks by either:\\nOverriding the `_checkImmutables()` function without the above checks.\\nCalling `Implementation::_setMigratesImmutables` with a `true` input. This effectively bypasses the `_checkImmutables()` function validation during an upgrade.\\nBased on the understanding that Transceivers are deployed by integrators external to NTT Manager owners, regardless of the high trust assumptions associated with integrators, it is risky for NTT Managers to delegate power to Transceivers to silently upgrade a transceiver contract that can potentially violate the NTT Manager invariants.\\nOne example of this involves the intended ownership model. Within `Transceiver::_initialize`, the owner of the Transceiver is set to the owner of the `NttManager` contract:\\n```\\nfunction _initialize() internal virtual override {\\n    // check if the owner is the deployer of this contract\\n    if (msg.sender != deployer) {\\n        revert UnexpectedDeployer(deployer, msg.sender);\\n    }\\n\\n    __ReentrancyGuard_init();\\n    // owner of the transceiver is set to the owner of the nttManager\\n    __PausedOwnable_init(msg.sender, getNttManagerOwner());\\n}\\n```\\n\\nHowever, the transferring of this ownership via `Transceiver::transferTransceiverOwnership` is only allowed by the NTT Manager itself:\\n```\\n/// @dev transfer the ownership of the transceiver to a new address\\n/// the nttManager should be able to update transceiver ownership.\\nfunction transferTransceiverOwnership(address newOwner) external onlyNttManager {\\n    _transferOwnership(newOwner);\\n}\\n```\\n\\nWhen the owner of the NTT Manager is changed by calling `NttManagerState::transferOwnership`, the owner of all the Transceivers is changed with it:\\n```\\n/// @notice Transfer ownership of the Manager contract and all Endpoint contracts to a new owner.\\nfunction transferOwnership(address newOwner) public override onlyOwner {\\n    super.transferOwnership(newOwner);\\n    // loop through all the registered transceivers and set the new owner of each transceiver to the newOwner\\n    address[] storage _registeredTransceivers = _getRegisteredTransceiversStorage();\\n    _checkRegisteredTransceiversInvariants();\\n\\n    for (uint256 i = 0; i < _registeredTransceivers.length; i++) {\\n        ITransceiver(_registeredTransceivers[i]).transferTransceiverOwnership(newOwner);\\n    }\\n}\\n```\\n\\nThis design is intended to ensure that the NTT Manager's owner is kept in sync across all transceivers, access-controlled to prevent unauthorized ownership changes, but transceiver ownership can still be transferred directly as the public `OwnableUpgradeable::transferOwnership` function has not been overridden. Even if Transceiver ownership changes, the Manager is permitted to change it again via the above function.\\nHowever, this behavior can be broken if the new owner of a Transceiver performs a contract upgrade without the immutables check. In this way, they can change the NTT Manager, preventing the correct manager from having permissions as expected. As a result, `NttManagerState::transferOwnership` will revert if any one Transceiver is out of sync with the others, and since it is not possible to remove an already registered transceiver, this function will cease to be useful. Instead, each Transceiver will be forced to be manually updated to the new owner unless the modified Transceiver is reset back to the previous owner so that this function can be called again.
Consider making `Transceiver::_checkImmutables` and `Implementation::_setMigratesImmutables` private functions for Transceivers. If the `_checkImmutables()` function has to be overridden, consider exposing another function that is called inside `_checkImmutables` as follows:\\n```\\nfunction _checkImmutables() private view override {\\n    assert(this.nttManager() == nttManager);\\n    assert(this.nttManagerToken() == nttManagerToken);\\n   _checkAdditionalImmutables();\\n}\\n\\nfunction _checkAdditionalImmutables() private view virtual override {}\\n```\\n
While this issue may require the owner of a Transceiver to misbehave, a scenario where a Transceiver is silently upgraded with a new NTT Manager or NTT Manager token can be problematic for cross-chain transfers and so is prescient to note.\\nProof of Concept: The below PoC calls the `_setMigratesImmutables()` function with the `true` boolean, effectively bypassing the `_checkImmutables()` invariant check. As a result, a subsequent call to `NttManagerState::transferOwnership` is demonstrated to revert. This test should be added to the contract in `Upgrades.t.sol` before running, and the revert in `MockWormholeTransceiverContract::transferOwnership` should be removed to reflect the `true` functionality.\\n```\\nfunction test_immutableUpgradePoC() public {\\n    // create the new mock ntt manager contract\\n    NttManager newImpl = new MockNttManagerContract(\\n        nttManagerChain1.token(), IManagerBase.Mode.BURNING, chainId1, 1 days, false\\n    );\\n    MockNttManagerContract newNttManager =\\n        MockNttManagerContract(address(new ERC1967Proxy(address(newImpl), "")));\\n    newNttManager.initialize();\\n\\n    // transfer transceiver ownership\\n    wormholeTransceiverChain1.transferOwnership(makeAddr("new transceiver owner"));\\n\\n    // create the new transceiver implementation, specifying the new ntt manager\\n    WormholeTransceiver wormholeTransceiverChain1Implementation = new MockWormholeTransceiverImmutableAllow(\\n        address(newNttManager),\\n        address(wormhole),\\n        address(relayer),\\n        address(0x0),\\n        FAST_CONSISTENCY_LEVEL,\\n        GAS_LIMIT\\n    );\\n\\n    // perform the transceiver upgrade\\n    wormholeTransceiverChain1.upgrade(address(wormholeTransceiverChain1Implementation));\\n\\n    // ntt manager ownership transfer should fail and revert\\n    vm.expectRevert(abi.encodeWithSelector(ITransceiver.CallerNotNttManager.selector, address(this)));\\n    nttManagerChain1.transferOwnership(makeAddr("new ntt manager owner"));\\n}\\n```\\n
```\\nfunction _initialize() internal virtual override {\\n    // check if the owner is the deployer of this contract\\n    if (msg.sender != deployer) {\\n        revert UnexpectedDeployer(deployer, msg.sender);\\n    }\\n\\n    __ReentrancyGuard_init();\\n    // owner of the transceiver is set to the owner of the nttManager\\n    __PausedOwnable_init(msg.sender, getNttManagerOwner());\\n}\\n```\\n