From 5a8010a2b87ff31c201b7966124a6402b42cf13d Mon Sep 17 00:00:00 2001 From: RnkSngh Date: Thu, 11 Apr 2024 12:39:43 -0500 Subject: [PATCH 1/2] fix writeTimeoutPacket vuln --- contracts/core/Dispatcher.sol | 6 +++++- contracts/interfaces/IDispatcher.sol | 2 ++ test/Dispatcher.proof.t.sol | 2 +- test/Dispatcher.t.sol | 20 ++++++++++++++++++- .../upgrades/DispatcherV2.sol | 12 +++++------ 5 files changed, 33 insertions(+), 9 deletions(-) diff --git a/contracts/core/Dispatcher.sol b/contracts/core/Dispatcher.sol index 1f862bce..f0645fac 100644 --- a/contracts/core/Dispatcher.sol +++ b/contracts/core/Dispatcher.sol @@ -474,7 +474,11 @@ contract Dispatcher is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { /** * Generate a timeout packet for the given packet */ - function writeTimeoutPacket(IbcPacket calldata packet) external { + function writeTimeoutPacket(IbcPacket calldata packet, Ics23Proof calldata proof) external { + _lightClient.verifyMembership( + proof, Ibc.packetCommitmentProofKey(packet), abi.encode(Ibc.packetCommitmentProofValue(packet)) + ); + address receiver = _getAddressFromPort(packet.src.portId); // verify packet does not have a receipt bool hasReceipt = _recvPacketReceipt[receiver][packet.dest.channelId][packet.sequence]; diff --git a/contracts/interfaces/IDispatcher.sol b/contracts/interfaces/IDispatcher.sol index ed156a89..9771beaa 100644 --- a/contracts/interfaces/IDispatcher.sol +++ b/contracts/interfaces/IDispatcher.sol @@ -89,6 +89,8 @@ interface IDispatcher is IbcDispatcher, IbcEventsEmitter { function timeout(IbcPacket calldata packet, Ics23Proof calldata proof) external; + function writeTimeoutPacket(IbcPacket calldata packet, Ics23Proof calldata proof) external; + function recvPacket(IbcPacket calldata packet, Ics23Proof calldata proof) external; function getOptimisticConsensusState(uint256 height) diff --git a/test/Dispatcher.proof.t.sol b/test/Dispatcher.proof.t.sol index f67cef34..c53da1b3 100644 --- a/test/Dispatcher.proof.t.sol +++ b/test/Dispatcher.proof.t.sol @@ -145,7 +145,7 @@ contract DispatcherIbcWithRealProofs is DispatcherIbcWithRealProofsSuite { (dispatcherProxy, dispatcherImplementation) = deployDispatcherProxyAndImpl(portPrefix1, consensusStateManager); address targetMarsAddress = 0x71C95911E9a5D330f4D621842EC243EE1343292e; - deployCodeTo("Mars.sol", abi.encode(address(dispatcherProxy)), targetMarsAddress); + deployCodeTo("Mars.sol:Mars", abi.encode(address(dispatcherProxy)), targetMarsAddress); mars = Mars(payable(targetMarsAddress)); portId1 = "polyibc.eth1.71C95911E9a5D330f4D621842EC243EE1343292e"; diff --git a/test/Dispatcher.t.sol b/test/Dispatcher.t.sol index 12cf3e54..d1967bc7 100644 --- a/test/Dispatcher.t.sol +++ b/test/Dispatcher.t.sol @@ -336,7 +336,7 @@ contract PacketSenderTestBase is ChannelOpenTestBaseSetup { // Test Chains B receives a packet from Chain A contract DispatcherRecvPacketTestSuite is ChannelOpenTestBaseSetup { - IbcEndpoint src = IbcEndpoint("polyibc.bsc.9876543210", "channel-99"); + IbcEndpoint src = IbcEndpoint("polyibc.bsc.58b604DB8886656695442374D8E940D814F2eDd4", "channel-99"); IbcEndpoint dest; bytes payload = bytes("msgPayload"); bytes appAck = abi.encodePacked('{ "account": "account", "reply": "got the message" }'); @@ -387,6 +387,24 @@ contract DispatcherRecvPacketTestSuite is ChannelOpenTestBaseSetup { dispatcherProxy.recvPacket(IbcPacket(src, dest, 3, payload, ZERO_HEIGHT, maxTimeout), validProof); } + // Can't spoof a timeed out packet + function test_writeTimeoutPacket_cannot_Spoof_Packet() public { + IbcPacket memory spoofedPacket = IbcPacket(src, dest, 1, payload, ZERO_HEIGHT, 1); + spoofedPacket.timeoutTimestamp = 1; // Set really low timeout timestamp + vm.mockCallRevert( + address(dummyConsStateManager), + abi.encodeWithSelector( + DummyLightClient.verifyMembership.selector, + validProof, + Ibc.packetCommitmentProofKey(spoofedPacket), + abi.encode(Ibc.packetCommitmentProofValue(spoofedPacket)) + ), + abi.encodeWithSelector(ProofVerifier.InvalidProofValue.selector) + ); + vm.expectRevert(abi.encodeWithSelector(ProofVerifier.InvalidProofValue.selector)); + dispatcherProxy.writeTimeoutPacket(spoofedPacket, validProof); + } + // TODO: add tests for unordered channel, wrong port, and invalid proof } diff --git a/test/upgradeableProxy/upgrades/DispatcherV2.sol b/test/upgradeableProxy/upgrades/DispatcherV2.sol index 1e435084..a56f552d 100644 --- a/test/upgradeableProxy/upgrades/DispatcherV2.sol +++ b/test/upgradeableProxy/upgrades/DispatcherV2.sol @@ -473,15 +473,15 @@ contract DispatcherV2 is OwnableUpgradeable, UUPSUpgradeable, IDispatcher { /** * Generate a timeout packet for the given packet */ - function writeTimeoutPacket(IbcPacket calldata packet) external { - // verify `receiver` is the original packet sender - // if (!portIdAddressMatch(receiver, packet.src.portId)) { - // revert IBCErrors.receiverNotIntendedPacketDestination(); - // } + function writeTimeoutPacket(IbcPacket calldata packet, Ics23Proof calldata proof) external { + _lightClient.verifyMembership( + proof, Ibc.packetCommitmentProofKey(packet), abi.encode(Ibc.packetCommitmentProofValue(packet)) + ); - address receiver = _getAddressFromPort(packet.dest.portId); + 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(); } From 787cbe3717a3c5c0ba33662be0705ac814e076c3 Mon Sep 17 00:00:00 2001 From: RnkSngh Date: Thu, 11 Apr 2024 16:39:26 -0500 Subject: [PATCH 2/2] cleanup code --- contracts/core/Dispatcher.sol | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/contracts/core/Dispatcher.sol b/contracts/core/Dispatcher.sol index f0645fac..aa22b5cd 100644 --- a/contracts/core/Dispatcher.sol +++ b/contracts/core/Dispatcher.sol @@ -454,23 +454,6 @@ contract Dispatcher 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 */ @@ -517,16 +500,6 @@ contract Dispatcher 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; - // } - // 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