Skip to content

Commit da692ad

Browse files
bluelebluele
authored
Merge pull request #286 from hyperledger-labs/fix-crossing-hello-proposal-validation
Fix to ensure `proposedConnectionHops` of MsgChannelUpgradeTry matches existing upgrade's one Signed-off-by: Jun Kimura <[email protected]>
2 parents 64ca8f4 + 9a7423e commit da692ad

File tree

4 files changed

+94
-45
lines changed

4 files changed

+94
-45
lines changed

‎.gas-snapshot‎

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
IBCMockAppTest:testHandshake() (gas: 4420488)
22
IBCMockAppTest:testHandshakeBetweenDifferentPorts() (gas: 3334373)
33
IBCMockAppTest:testPacketRelay() (gas: 13935239)
4-
IBCMockAppTest:testPacketTimeout() (gas: 4279263)
4+
IBCMockAppTest:testPacketTimeout() (gas: 4284263)
55
IBCTest:testBenchmarkCreateMockClient() (gas: 233366)
66
IBCTest:testBenchmarkLCUpdateMockClient() (gas: 62005)
77
IBCTest:testBenchmarkRecvPacket() (gas: 158870)
88
IBCTest:testBenchmarkSendPacket() (gas: 128432)
99
IBCTest:testBenchmarkUpdateMockClient() (gas: 160229)
1010
IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 947, ~: 947)
11-
TestICS02:testCreateClient() (gas: 36540928)
12-
TestICS02:testInvalidCreateClient() (gas: 36438147)
13-
TestICS02:testInvalidUpdateClient() (gas: 36437055)
14-
TestICS02:testRegisterClient() (gas: 36092721)
15-
TestICS02:testRegisterClientDuplicatedClientType() (gas: 36078030)
16-
TestICS02:testRegisterClientInvalidClientType() (gas: 36107491)
17-
TestICS02:testUpdateClient() (gas: 36605255)
11+
TestICS02:testCreateClient() (gas: 36576736)
12+
TestICS02:testInvalidCreateClient() (gas: 36473955)
13+
TestICS02:testInvalidUpdateClient() (gas: 36472863)
14+
TestICS02:testRegisterClient() (gas: 36128529)
15+
TestICS02:testRegisterClientDuplicatedClientType() (gas: 36113838)
16+
TestICS02:testRegisterClientInvalidClientType() (gas: 36143299)
17+
TestICS02:testUpdateClient() (gas: 36641063)
1818
TestICS03Handshake:testConnOpenAck() (gas: 1858230)
1919
TestICS03Handshake:testConnOpenConfirm() (gas: 2054143)
2020
TestICS03Handshake:testConnOpenInit() (gas: 1429838)
@@ -30,7 +30,7 @@ TestICS03Version:testPickVersion() (gas: 25327)
3030
TestICS03Version:testVerifyProposedVersion() (gas: 11777)
3131
TestICS03Version:testVerifySupportedFeature() (gas: 4153)
3232
TestICS04Handshake:testBindPort() (gas: 124350)
33-
TestICS04Handshake:testChanClose() (gas: 12938854)
33+
TestICS04Handshake:testChanClose() (gas: 12973854)
3434
TestICS04Handshake:testChanOpenAck() (gas: 3459404)
3535
TestICS04Handshake:testChanOpenConfirm() (gas: 3770673)
3636
TestICS04Handshake:testChanOpenInit() (gas: 2543524)
@@ -40,34 +40,34 @@ TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2517338)
4040
TestICS04Handshake:testInvalidChanOpenInit() (gas: 1760558)
4141
TestICS04Handshake:testInvalidChanOpenTry() (gas: 1775528)
4242
TestICS04Packet:testAcknowledgementPacket() (gas: 3351116)
43-
TestICS04Packet:testInvalidSendPacket() (gas: 3551583)
44-
TestICS04Packet:testRecvPacket() (gas: 10996942)
43+
TestICS04Packet:testInvalidSendPacket() (gas: 3579083)
44+
TestICS04Packet:testRecvPacket() (gas: 11006942)
4545
TestICS04Packet:testRecvPacketTimeoutHeight() (gas: 3259727)
46-
TestICS04Packet:testRecvPacketTimeoutTimestamp() (gas: 3279103)
46+
TestICS04Packet:testRecvPacketTimeoutTimestamp() (gas: 3284103)
4747
TestICS04Packet:testSendPacket() (gas: 6412442)
4848
TestICS04Packet:testTimeoutOnClose() (gas: 3553289)
49-
TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46742335)
49+
TestICS04Upgrade:testUpgradeAuthorityCancel() (gas: 46744545)
5050
TestICS04Upgrade:testUpgradeCannotCancelWithOldErrorReceipt() (gas: 3456630)
51-
TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5266005)
52-
TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236258)
53-
TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4407259)
54-
TestICS04Upgrade:testUpgradeFull() (gas: 57784728)
55-
TestICS04Upgrade:testUpgradeInit() (gas: 3069408)
51+
TestICS04Upgrade:testUpgradeCannotRecvNextUpgradePacket() (gas: 5265976)
52+
TestICS04Upgrade:testUpgradeCounterpartyAdvanceNextSequenceBeforeOpen() (gas: 5236229)
53+
TestICS04Upgrade:testUpgradeCrossingHelloIncompatibleProposals() (gas: 4996384)
54+
TestICS04Upgrade:testUpgradeFull() (gas: 56608897)
55+
TestICS04Upgrade:testUpgradeInit() (gas: 3071908)
5656
TestICS04Upgrade:testUpgradeNoChanges() (gas: 2471936)
57-
TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3903220)
58-
TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5237770)
59-
TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5610957)
60-
TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4071025)
61-
TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17694261)
62-
TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21333518)
63-
TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 71049040)
64-
TestICS04Upgrade:testUpgradeToOrdered() (gas: 56509529)
65-
TestICS04Upgrade:testUpgradeToUnordered() (gas: 45113829)
57+
TestICS04Upgrade:testUpgradeOutOfSync() (gas: 3904992)
58+
TestICS04Upgrade:testUpgradeRelaySuccessAtCounterpartyFlushComplete() (gas: 5237741)
59+
TestICS04Upgrade:testUpgradeRelaySuccessAtFlushing() (gas: 5610928)
60+
TestICS04Upgrade:testUpgradeSendPacketFailAtFlushingOrFlushComplete() (gas: 4070996)
61+
TestICS04Upgrade:testUpgradeTimeoutAbortAck() (gas: 17693913)
62+
TestICS04Upgrade:testUpgradeTimeoutAbortConfirm() (gas: 21333170)
63+
TestICS04Upgrade:testUpgradeTimeoutUpgrade() (gas: 71048576)
64+
TestICS04Upgrade:testUpgradeToOrdered() (gas: 56509239)
65+
TestICS04Upgrade:testUpgradeToUnordered() (gas: 45113597)
6666
TestICS04UpgradeApp:testUpgradeAuthorizationChanneNotFound() (gas: 61690)
6767
TestICS04UpgradeApp:testUpgradeAuthorizationRePropose() (gas: 2565532)
68-
TestICS04UpgradeApp:testUpgradeAuthorizationRemove() (gas: 2473992)
69-
TestICS20:testAddressToHex(address) (runs: 256, μ: 22726, ~: 22867)
68+
TestICS04UpgradeApp:testUpgradeAuthorizationRemove() (gas: 2475992)
69+
TestICS20:testAddressToHex(address) (runs: 256, μ: 22668, ~: 22804)
7070
TestICS20:testHexToAddress(string) (runs: 256, μ: 4776, ~: 4734)
7171
TestICS20:testIsEscapedString() (gas: 48979)
7272
TestICS20:testMarshaling() (gas: 148517)
73-
TestICS20:testParseAmount(uint256) (runs: 256, μ: 26449, ~: 24175)
73+
TestICS20:testParseAmount(uint256) (runs: 256, μ: 26349, ~: 23311)

‎contracts/core/04-channel/IBCChannelUpgrade.sol‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -457,10 +457,14 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
457457
// nextSequenceSend and timeout will be filled when we move to FLUSHING
458458
existingUpgrade.fields = upgradeFields;
459459
} else {
460-
if (msg_.proposedConnectionHops.length != 0) {
461-
revert IBCChannelUpgradeTryProposalMustEmptyIfExist();
460+
if (msg_.proposedConnectionHops.length != 1) {
461+
revert IBCChannelUpgradeTryProposedConnectionHopsEmpty();
462462
}
463463
upgradeFields = existingUpgrade.fields;
464+
if (keccak256(bytes(upgradeFields.connection_hops[0])) != keccak256(bytes(msg_.proposedConnectionHops[0])))
465+
{
466+
revert IBCChannelUpgradeTryProposedConnectionHopsMismatch();
467+
}
464468
}
465469

466470
if (!isCompatibleUpgradeFields(upgradeFields, msg_.counterpartyUpgradeFields)) {

‎contracts/core/04-channel/IIBCChannelUpgradeErrors.sol‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,11 @@ interface IIBCChannelUpgradeErrors is IIBCChannelErrors {
1010

1111
error IBCChannelUpgradeInvalidUpgradeFields();
1212

13-
/// @notice proposal must be empty if upgrade exists
14-
error IBCChannelUpgradeTryProposalMustEmptyIfExist();
13+
/// @notice proposed upgrade must be non-empty
14+
error IBCChannelUpgradeTryProposedConnectionHopsEmpty();
15+
16+
/// @notice proposal's connection hops mismatch with the existing proposal
17+
error IBCChannelUpgradeTryProposedConnectionHopsMismatch();
1518

1619
/// @param upgrader address of the upgrader
1720
error IBCChannelUpgradeUnauthorizedChannelUpgrader(address upgrader);

‎tests/foundry/src/ICS04Upgrade.t.sol‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
161161
channelId: channel0.channelId,
162162
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel1.portId, channel1.channelId).fields,
163163
counterpartyUpgradeSequence: 2,
164-
proposedConnectionHops: new string[](0),
164+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel0.connectionId),
165165
proofs: upgradeLocalhostProofs()
166166
});
167167
(bool ok, uint64 seq) = ibcHandler.channelUpgradeTry(msg_);
@@ -217,6 +217,30 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
217217
);
218218

219219
// Try msg is reverted because the proposals are incompatible
220+
{
221+
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
222+
portId: channel0.portId,
223+
channelId: channel0.channelId,
224+
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel1.portId, channel1.channelId).fields,
225+
counterpartyUpgradeSequence: 1,
226+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel0.connectionId),
227+
proofs: upgradeLocalhostProofs()
228+
});
229+
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeIncompatibleProposal.selector);
230+
ibcHandler.channelUpgradeTry(msg_);
231+
}
232+
{
233+
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
234+
portId: channel0.portId,
235+
channelId: channel0.channelId,
236+
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel1.portId, channel1.channelId).fields,
237+
counterpartyUpgradeSequence: 1,
238+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel1.connectionId),
239+
proofs: upgradeLocalhostProofs()
240+
});
241+
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeTryProposedConnectionHopsMismatch.selector);
242+
ibcHandler.channelUpgradeTry(msg_);
243+
}
220244
{
221245
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
222246
portId: channel0.portId,
@@ -226,9 +250,33 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
226250
proposedConnectionHops: new string[](0),
227251
proofs: upgradeLocalhostProofs()
228252
});
253+
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeTryProposedConnectionHopsEmpty.selector);
254+
ibcHandler.channelUpgradeTry(msg_);
255+
}
256+
{
257+
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
258+
portId: channel1.portId,
259+
channelId: channel1.channelId,
260+
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel0.portId, channel0.channelId).fields,
261+
counterpartyUpgradeSequence: 1,
262+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel1.connectionId),
263+
proofs: upgradeLocalhostProofs()
264+
});
229265
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeIncompatibleProposal.selector);
230266
ibcHandler.channelUpgradeTry(msg_);
231267
}
268+
{
269+
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
270+
portId: channel1.portId,
271+
channelId: channel1.channelId,
272+
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel0.portId, channel0.channelId).fields,
273+
counterpartyUpgradeSequence: 1,
274+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel0.connectionId),
275+
proofs: upgradeLocalhostProofs()
276+
});
277+
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeTryProposedConnectionHopsMismatch.selector);
278+
ibcHandler.channelUpgradeTry(msg_);
279+
}
232280
{
233281
IIBCChannelUpgradeBase.MsgChannelUpgradeTry memory msg_ = IIBCChannelUpgradeBase.MsgChannelUpgradeTry({
234282
portId: channel1.portId,
@@ -238,7 +286,7 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
238286
proposedConnectionHops: new string[](0),
239287
proofs: upgradeLocalhostProofs()
240288
});
241-
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeIncompatibleProposal.selector);
289+
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeTryProposedConnectionHopsEmpty.selector);
242290
ibcHandler.channelUpgradeTry(msg_);
243291
}
244292

@@ -265,7 +313,7 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
265313
channelId: channel0.channelId,
266314
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel1.portId, channel1.channelId).fields,
267315
counterpartyUpgradeSequence: 1,
268-
proposedConnectionHops: new string[](0),
316+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel0.connectionId),
269317
proofs: upgradeLocalhostProofs()
270318
});
271319
vm.expectRevert(
@@ -283,7 +331,7 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
283331
channelId: channel1.channelId,
284332
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel0.portId, channel0.channelId).fields,
285333
counterpartyUpgradeSequence: 2,
286-
proposedConnectionHops: new string[](0),
334+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel1.connectionId),
287335
proofs: upgradeLocalhostProofs()
288336
});
289337
vm.expectRevert(IIBCChannelUpgradeErrors.IBCChannelUpgradeIncompatibleProposal.selector);
@@ -312,7 +360,7 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
312360
channelId: channel0.channelId,
313361
counterpartyUpgradeFields: mockApp.getUpgradeProposal(channel1.portId, channel1.channelId).fields,
314362
counterpartyUpgradeSequence: 2,
315-
proposedConnectionHops: new string[](0),
363+
proposedConnectionHops: IBCChannelLib.buildConnectionHops(channel0.connectionId),
316364
proofs: upgradeLocalhostProofs()
317365
})
318366
);
@@ -1210,12 +1258,6 @@ contract TestICS04Upgrade is ICS04UpgradeTestHelper, ICS04PacketEventTestHelper
12101258
proposedConnectionHops: IBCChannelLib.buildConnectionHops(proposals.p1.connectionId),
12111259
proofs: upgradeLocalhostProofs()
12121260
});
1213-
if (flow.crossingHello) {
1214-
// If the upgrade already exists (i.e. crossing hello), `proposedConnectionHops` must be empty
1215-
vm.expectRevert();
1216-
ibcHandler.channelUpgradeTry(msg_);
1217-
msg_.proposedConnectionHops = new string[](0);
1218-
}
12191261
(bool ok, uint64 seq) = ibcHandler.channelUpgradeTry(msg_);
12201262
assertTrue(ok);
12211263
assertEq(seq, upgradeSequence);

0 commit comments

Comments
 (0)