diff --git a/contracts/core/Dispatcher.sol b/contracts/core/Dispatcher.sol index 0979cb7d..26a489f9 100644 --- a/contracts/core/Dispatcher.sol +++ b/contracts/core/Dispatcher.sol @@ -247,31 +247,16 @@ contract Dispatcher is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { } /** - * @dev Emits a `CloseIbcChannel` event with the given `channelId` and the address of the message sender - * @notice Close the specified IBC channel by channel ID - * Must be called by the channel owner, ie. _portChannelMap[msg.sender][channelId] must exist + * @notice Initializes a close channel handshake process. It is directly called by the dapp which wants to close + * the channel */ - function closeIbcChannel(bytes32 channelId) external { - Channel memory channel = _portChannelMap[msg.sender][channelId]; - if (channel.counterpartyChannelId == bytes32(0)) { - revert IBCErrors.channelNotOwnedBySender(); - } + function channelCloseInit(bytes32 channelId) external {} - (bool success, bytes memory data) = _callIfContract( - msg.sender, - abi.encodeWithSelector( - IbcChannelReceiver.onCloseIbcChannel.selector, - channelId, - channel.counterpartyPortId, - channel.counterpartyChannelId - ) - ); - if (success) { - emit CloseIbcChannel(msg.sender, channelId); - } else { - emit CloseIbcChannelError(address(msg.sender), data); - } - } + /** + * @notice Confirms a close channel handshake process. It is called by a relayer on behalf of the dapp whhich + * initializes the channel closefter after the IBC/VIBC hub chain has processed ChanCloseConfirm event. + */ + function channelCloseConfirm(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external {} /** * This func is called by a 'relayer' after the IBC/VIBC hub chain has processed ChanCloseConfirm event. diff --git a/contracts/core/UniversalChannelHandler.sol b/contracts/core/UniversalChannelHandler.sol index d1fcbd23..245283d9 100644 --- a/contracts/core/UniversalChannelHandler.sol +++ b/contracts/core/UniversalChannelHandler.sol @@ -14,8 +14,9 @@ import { import {IbcReceiver} from "../interfaces/IbcReceiver.sol"; import {IbcReceiverBaseUpgradeable} from "../interfaces/IbcReceiverUpgradeable.sol"; import {ChannelOrder, ChannelEnd, IbcPacket, AckPacket, UniversalPacket, IbcUtils} from "../libs/Ibc.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; -contract UniversalChannelHandler is IbcReceiverBaseUpgradeable, IbcUniversalChannelMW { +contract UniversalChannelHandler is IbcReceiverBaseUpgradeable, UUPSUpgradeable, IbcUniversalChannelMW { uint256[49] private __gap; bytes32[] public connectedChannels; @@ -34,28 +35,10 @@ contract UniversalChannelHandler is IbcReceiverBaseUpgradeable, IbcUniversalChan function initialize(IbcDispatcher _dispatcher) public initializer { __IbcReceiverBase_init(_dispatcher); } - /** - * @dev Close a universal channel. - * Cannot send or receive packets after the channel is closed. - * @param channelId The channel id of the channel to be closed. - */ - function closeChannel(bytes32 channelId) external onlyOwner { - dispatcher.closeIbcChannel(channelId); - } + function onChanCloseInit(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {} - function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher { - // logic to determin if the channel should be closed - bool channelFound = false; - for (uint256 i = 0; i < connectedChannels.length; i++) { - if (connectedChannels[i] == channelId) { - delete connectedChannels[i]; - channelFound = true; - break; - } - } - if (!channelFound) revert ChannelNotFound(); - } + function onChanCloseConfirm(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {} function openChannel( string calldata version, @@ -195,6 +178,8 @@ contract UniversalChannelHandler is IbcReceiverBaseUpgradeable, IbcUniversalChan dispatcher = _dispatcher; } + function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + function _connectChannel(bytes32 channelId, string calldata version) internal returns (string memory checkedVersion) diff --git a/contracts/examples/Mars.sol b/contracts/examples/Mars.sol index 695d6b95..1b1f7455 100644 --- a/contracts/examples/Mars.sol +++ b/contracts/examples/Mars.sol @@ -49,27 +49,6 @@ contract Mars is IbcReceiverBase, IbcReceiver { timeoutPackets.push(packet); } - function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external virtual onlyIbcDispatcher { - // logic to determin if the channel should be closed - bool channelFound = false; - for (uint256 i = 0; i < connectedChannels.length; i++) { - if (connectedChannels[i] == channelId) { - delete connectedChannels[i]; - channelFound = true; - break; - } - } - if (!channelFound) revert ChannelNotFound(); - } - - /** - * This func triggers channel closure from the dApp. - * Func args can be arbitary, as long as dispatcher.closeIbcChannel is invoked propperly. - */ - function triggerChannelClose(bytes32 channelId) external onlyOwner { - dispatcher.closeIbcChannel(channelId); - } - /** * @dev Sends a packet with a greeting message over a specified channel. * @param message The greeting message to be sent. @@ -134,6 +113,10 @@ contract Mars is IbcReceiverBase, IbcReceiver { } revert UnsupportedVersion(); } + + function onChanCloseInit(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {} + + function onChanCloseConfirm(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {} } /* @@ -170,12 +153,6 @@ contract RevertingStringMars is Mars { require(false, "connect ibc channel is reverting"); } - // solhint-disable-next-line - function onCloseIbcChannel(bytes32, string calldata, bytes32) external view override onlyIbcDispatcher { - // solhint-disable-next-line - require(false, "close ibc channel is reverting"); - } - // solhint-disable-next-line function onAcknowledgementPacket(IbcPacket calldata, AckPacket calldata) external view override onlyIbcDispatcher { // solhint-disable-next-line diff --git a/contracts/interfaces/IDispatcher.sol b/contracts/interfaces/IDispatcher.sol index 9771beaa..c0df9782 100644 --- a/contracts/interfaces/IDispatcher.sol +++ b/contracts/interfaces/IDispatcher.sol @@ -81,7 +81,8 @@ interface IDispatcher is IbcDispatcher, IbcEventsEmitter { Ics23Proof calldata proof ) external; - function closeIbcChannel(bytes32 channelId) external; + function channelCloseConfirm(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external; + function channelCloseInit(bytes32 channelId) external; function sendPacket(bytes32 channelId, bytes calldata packet, uint64 timeoutTimestamp) external; diff --git a/contracts/interfaces/IbcDispatcher.sol b/contracts/interfaces/IbcDispatcher.sol index d853acdb..2ede4d34 100644 --- a/contracts/interfaces/IbcDispatcher.sol +++ b/contracts/interfaces/IbcDispatcher.sol @@ -31,7 +31,8 @@ interface IbcDispatcher is IbcPacketSender { string calldata counterpartyPortId ) external; - function closeIbcChannel(bytes32 channelId) external; + function channelCloseConfirm(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external; + function channelCloseInit(bytes32 channelId) external; function portPrefix() external view returns (string memory portPrefix); } diff --git a/contracts/interfaces/IbcReceiver.sol b/contracts/interfaces/IbcReceiver.sol index a3de6c46..f76ffeef 100644 --- a/contracts/interfaces/IbcReceiver.sol +++ b/contracts/interfaces/IbcReceiver.sol @@ -32,11 +32,25 @@ interface IbcChannelReceiver { external; function onChanOpenConfirm(bytes32 channelId) external; - function onCloseIbcChannel( - bytes32 channelId, - string calldata counterpartyPortIdentifier, - bytes32 counterpartyChannelId - ) external; + /** + * @notice Handles the channel close init event + * @param channelId The unique identifier of the channel + * @dev Make sure to validate channelId and counterpartyVersion + * @param counterpartyPortId The unique identifier of the counterparty's port + * @param counterpartyChannelId The unique identifier of the counterparty's channel + */ + function onChanCloseInit(bytes32 channelId, string calldata counterpartyPortId, bytes32 counterpartyChannelId) + external; + + /** + * @notice Handles the channel close confirmation event + * @param channelId The unique identifier of the channel + * @dev Make sure to validate channelId and counterpartyVersion + * @param counterpartyPortId The unique identifier of the counterparty's port + * @param counterpartyChannelId The unique identifier of the counterparty's channel + */ + function onChanCloseConfirm(bytes32 channelId, string calldata counterpartyPortId, bytes32 counterpartyChannelId) + external; } /** diff --git a/test/universal.channel.t.sol b/test/universal.channel.t.sol index 37a0e571..ffd37155 100644 --- a/test/universal.channel.t.sol +++ b/test/universal.channel.t.sol @@ -245,6 +245,31 @@ contract UniversalChannelPacketTest is Base, IbcMwEventsEmitter { vm.stopPrank(); } + function test_uch_uch_upgrade__ok() public { + IUniversalChannelHandler uch = eth1.ucHandlerProxy(); + UniversalChannelHandler newUCHImplementation = new UniversalChannelHandler(); + vm.startPrank(address(eth1)); // Prank eth1 since that address is the owner + assertFalse(address(getProxyImplementation(address(uch), vm)) == address(newUCHImplementation)); + UUPSUpgradeable(address(uch)).upgradeTo(address(address(newUCHImplementation))); + + assertEq( + address(getProxyImplementation(address(uch), vm)), + address(newUCHImplementation), + "new uch implementation not set correctly in uch proxy" + ); + vm.stopPrank(); + } + + function test_nonOwner_cannot_upgrade_uch() public { + IUniversalChannelHandler uch = eth1.ucHandlerProxy(); + UniversalChannelHandler newUCHImplementation = new UniversalChannelHandler(); + address notOwner = vm.addr(1); + vm.startPrank(notOwner); + vm.expectRevert("Ownable: caller is not the owner"); + UUPSUpgradeable(address(uch)).upgradeTo(address(address(newUCHImplementation))); + vm.stopPrank(); + } + /** * Test packet flow from chain A to chain B via UniversalChannel MW and optionally other MW that sits on top of * UniversalChannel MW. diff --git a/test/upgradeableProxy/upgrades/DispatcherV2.sol b/test/upgradeableProxy/upgrades/DispatcherV2.sol index a56f552d..f7a7bb26 100644 --- a/test/upgradeableProxy/upgrades/DispatcherV2.sol +++ b/test/upgradeableProxy/upgrades/DispatcherV2.sol @@ -112,8 +112,10 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { revert IBCErrors.invalidCounterPartyPortId(); } + // Have to encode here to avoid stack-too-deep error + bytes memory chanOpenInitArgs = abi.encode(ordering, connectionHops, counterpartyPortId, version); (bool success, bytes memory data) = - _callIfContract(msg.sender, abi.encodeWithSelector(IbcChannelReceiver.onChanOpenInit.selector, version)); + _callIfContract(msg.sender, bytes.concat(IbcChannelReceiver.onChanOpenInit.selector, chanOpenInitArgs)); if (success) { emit ChannelOpenInit( @@ -149,7 +151,16 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { address receiver = _getAddressFromPort(local.portId); (bool success, bytes memory data) = _callIfContract( - receiver, abi.encodeWithSelector(IbcChannelReceiver.onChanOpenTry.selector, counterparty.version) + receiver, + abi.encodeWithSelector( + IbcChannelReceiver.onChanOpenTry.selector, + ordering, + connectionHops, + local.channelId, + counterparty.portId, + counterparty.channelId, + counterparty.version + ) ); if (success) { @@ -188,7 +199,9 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { address receiver = _getAddressFromPort(local.portId); (bool success, bytes memory data) = _callIfContract( receiver, - abi.encodeWithSelector(IbcChannelReceiver.onChanOpenAck.selector, local.channelId, counterparty.version) + abi.encodeWithSelector( + IbcChannelReceiver.onChanOpenAck.selector, local.channelId, counterparty.channelId, counterparty.version + ) ); if (success) { @@ -233,31 +246,16 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { } /** - * @dev Emits a `CloseIbcChannel` event with the given `channelId` and the address of the message sender - * @notice Close the specified IBC channel by channel ID - * Must be called by the channel owner, ie. _portChannelMap[msg.sender][channelId] must exist + * @notice Initializes a close channel handshake process. It is directly called by the dapp which wants to close + * the channel */ - function closeIbcChannel(bytes32 channelId) external { - Channel memory channel = _portChannelMap[msg.sender][channelId]; - if (channel.counterpartyChannelId == bytes32(0)) { - revert IBCErrors.channelNotOwnedBySender(); - } + function channelCloseInit(bytes32 channelId) external {} - (bool success, bytes memory data) = _callIfContract( - msg.sender, - abi.encodeWithSelector( - IbcChannelReceiver.onCloseIbcChannel.selector, - channelId, - channel.counterpartyPortId, - channel.counterpartyChannelId - ) - ); - if (success) { - emit CloseIbcChannel(msg.sender, channelId); - } else { - emit CloseIbcChannelError(address(msg.sender), data); - } - } + /** + * @notice Confirms a close channel handshake process. It is called by a relayer on behalf of the dapp whhich + * initializes the channel closefter after the IBC/VIBC hub chain has processed ChanCloseConfirm event. + */ + function channelCloseConfirm(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external {} /** * This func is called by a 'relayer' after the IBC/VIBC hub chain has processed ChanCloseConfirm event. @@ -281,7 +279,8 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { // // // confirm with dApp by calling its callback // IbcChannelReceiver reciever = IbcChannelReceiver(portAddress); - // reciever.onCloseIbcChannel(channelId, channel.counterpartyPortId, channel.counterpartyChannelId); + // reciever.onCloseIbcChannel(channelId, channel.counterpartyPortId, + // channel.counterpartyChannelId); // delete _portChannelMap[portAddress][channelId]; // emit CloseIbcChannel(portAddress, channelId); // } @@ -453,23 +452,6 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { emit WriteAckPacket(receiver, packet.dest.channelId, packet.sequence, ack); } - // TODO: add async writeAckPacket - // // this can be invoked sync or async by the IBC-dApp - // function writeAckPacket(IbcPacket calldata packet, AckPacket calldata ackPacket) external { - // // verify `receiver` is the original packet sender - // require( - // portIdAddressMatch(address(msg.sender), packet.src.portId), - // 'Receiver is not the original packet sender' - // ); - // } - - // TODO: remove below writeTimeoutPacket() function - // 1. core SC is responsible to generate timeout packet - // 2. user contract are not free to generate timeout with different criteria - // 3. [optional]: we may wish relayer to trigger timeout process, but in this case, belowunction won't do - // the job, as it doesn't have proofs. - // There is no strong reason to do this, as relayer can always do the regular `recvPacket` flow, which will - // do proper timeout generation. /** * Generate a timeout packet for the given packet */ @@ -481,7 +463,6 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { address receiver = _getAddressFromPort(packet.src.portId); // verify packet does not have a receipt bool hasReceipt = _recvPacketReceipt[receiver][packet.dest.channelId][packet.sequence]; - if (hasReceipt) { revert IBCErrors.packetReceiptAlreadyExists(); } @@ -517,20 +498,6 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { return _lightClient.getState(height); } - // verify an EVM address matches an IBC portId. - // IBC_PortID = portPrefix + address (hex string without 0x prefix, case-insensitive) - // function portIdAddressMatch(address addr, string calldata portId) public view returns (bool isMatch) { - // if (keccak256(abi.encodePacked(portPrefix)) != keccak256(abi.encodePacked(portId[0:portPrefixLen]))) { - // return false; - // } - // string memory portSuffix = portId[portPrefixLen:]; - // isMatch = Ibc._hexStrToAddress(portSuffix) == addr; - // } - - function _getAddressFromPort(string calldata port) internal view returns (address) { - return Ibc._hexStrToAddress(port[portPrefixLen:]); - } - // Prerequisite: must verify sender is authorized to send packet on the channel function _sendPacket(address sender, bytes32 channelId, bytes memory packet, uint64 timeoutTimestamp) internal { // current packet sequence @@ -572,7 +539,8 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { _nextSequenceSend[address(portAddress)][local.channelId] = 1; _nextSequenceRecv[address(portAddress)][local.channelId] = 1; _nextSequenceAck[address(portAddress)][local.channelId] = 1; - _channelIdToConnection[local.channelId] = connectionHops[0]; // Set channel to connection mapping for finding + _channelIdToConnection[local.channelId] = connectionHops[0]; // Set channel to connection mapping for + // finding } // Returns the result of the call if no revert, otherwise returns the error if thrown. @@ -600,4 +568,8 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { || (packet.timeoutHeight.revision_height != 0 && block.number >= packet.timeoutHeight.revision_height) ); } + + function _getAddressFromPort(string calldata port) internal view returns (address addr) { + addr = Ibc._hexStrToAddress(port[portPrefixLen:]); + } }