Skip to content

Commit 3e93661

Browse files
AntonAndellAntonAndell
committed
feat: Apply code improvments suggested during audit
1 parent b0912f6 commit 3e93661

File tree

7 files changed

+173
-167
lines changed

7 files changed

+173
-167
lines changed

‎contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -290,11 +290,15 @@ public void _timeoutPacket(Packet packet, byte[] proofHeight, byte[] proof, BigI
290290
ConnectionEnd connection = ConnectionEnd.decode(connectionPb);
291291

292292
// check that timeout height or timeout timestamp has passed on the other end
293+
ILightClient client = getClient(connection.getClientId());
293294
Height height = Height.decode(proofHeight);
295+
BigInteger timestamp = client.getTimestampAtHeight(connection.getClientId(), proofHeight);
294296
boolean heightTimeout = packet.getTimeoutHeight().getRevisionHeight().compareTo(BigInteger.ZERO) > 0
295297
&& height.getRevisionHeight()
296298
.compareTo(packet.getTimeoutHeight().getRevisionHeight()) >= 0;
297-
Context.require(heightTimeout, "Packet has not yet timed out");
299+
boolean timeTimeout = packet.getTimeoutTimestamp().compareTo(BigInteger.ZERO) > 0
300+
&& timestamp.compareTo(packet.getTimeoutTimestamp()) >= 0;
301+
Context.require(heightTimeout || timeTimeout, "Packet has not yet timed out");
298302

299303
// verify we actually sent this packet, check the store
300304
byte[] packetCommitmentKey = IBCCommitment.packetCommitmentKey(packet.getSourcePort(),

‎contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java

+85-71
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
package ibc.ics08.tendermint;
22

33
import icon.ibc.interfaces.ILightClient;
4-
import ibc.icon.score.util.ByteUtil;
54
import ibc.icon.score.util.NullChecker;
65
import ibc.icon.score.util.StringUtil;
76
import ibc.ics23.commitment.types.Merkle;
87
import ibc.ics24.host.IBCCommitment;
9-
import cosmos.ics23.v1.*;
108
import google.protobuf.*;
119
import tendermint.types.*;
1210
import ibc.core.commitment.v1.*;
@@ -17,7 +15,6 @@
1715
import score.Context;
1816
import score.DictDB;
1917
import score.annotation.External;
20-
import score.annotation.Optional;
2118

2219
import java.math.BigInteger;
2320
import java.util.Arrays;
@@ -54,7 +51,7 @@ private void onlyHandler() {
5451

5552
/**
5653
* @dev getTimestampAtHeight returns the timestamp of the consensus state at the
57-
* given height.
54+
* given height.
5855
*/
5956
@External(readonly = true)
6057
public BigInteger getTimestampAtHeight(
@@ -118,79 +115,29 @@ public Map<String, byte[]> createClient(String clientId, byte[] clientStateBytes
118115
@External
119116
public Map<String, byte[]> updateClient(String clientId, byte[] clientMessageBytes) {
120117
onlyHandler();
121-
ibc.lightclients.tendermint.v1.Header tmHeader = ibc.lightclients.tendermint.v1.Header.decode(clientMessageBytes);
122-
boolean conflictingHeader = false;
123-
// Check if the Client store already has a consensus state for the header's
124-
// height
125-
// If the consensus state exists, and it matches the header then we return early
126-
// since header has already been submitted in a previous UpdateClient.
127-
byte[] prevConsState = consensusStates.at(clientId)
128-
.get(tmHeader.getSignedHeader().getHeader().getHeight());
129-
if (prevConsState != null) {
130-
// This header has already been submitted and the necessary state is already
131-
// stored
132-
Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()),
133-
"LC: This header has already been submitted");
134-
135-
// A consensus state already exists for this height, but it does not match the
136-
// provided header.
137-
// Thus, we must check that this header is valid, and if so we will freeze the
138-
// client.
139-
conflictingHeader = true;
140-
}
118+
ibc.lightclients.tendermint.v1.Header tmHeader = ibc.lightclients.tendermint.v1.Header
119+
.decode(clientMessageBytes);
120+
boolean conflictingHeader = checkForDuplicateHeader(clientId, tmHeader);
141121

142122
byte[] encodedClientState = clientStates.get(clientId);
143123
require(encodedClientState != null, "LC: client state is invalid");
144124
ClientState clientState = ClientState.decode(encodedClientState);
145-
byte[] encodedTrustedonsensusState = consensusStates.at(clientId).get(tmHeader.getTrustedHeight().getRevisionHeight());
146-
require(encodedTrustedonsensusState != null, "LC: consensusState not found at trusted height");
147-
ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedonsensusState);
148125

149-
Timestamp currentTime = getCurrentTime();
150-
checkValidity(clientState, trustedConsensusState, tmHeader, currentTime);
126+
byte[] encodedTrustedConsensusState = consensusStates.at(clientId)
127+
.get(tmHeader.getTrustedHeight().getRevisionHeight());
128+
require(encodedTrustedConsensusState != null, "LC: consensusState not found at trusted height");
129+
ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedConsensusState);
130+
131+
checkValidity(clientState, trustedConsensusState, tmHeader);
151132

152133
// Header is different from existing consensus state and also valid, so freeze
153134
// the client and return
154135
if (conflictingHeader) {
155-
BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId());
156-
clientState.setFrozenHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision));
157-
encodedClientState = clientState.encode();
158-
clientStates.set(clientId, encodedClientState);
159-
160-
byte[] encodedConsensusState = toConsensusState(tmHeader).encode();
161-
consensusStates.at(clientId).set(clientState.getLatestHeight().getRevisionHeight(), encodedConsensusState);
162-
processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
163-
BigInteger.valueOf(Context.getBlockHeight()));
164-
processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
165-
BigInteger.valueOf(Context.getBlockTimestamp()));
166-
167-
return Map.of(
168-
"clientStateCommitment", IBCCommitment.keccak256(encodedClientState),
169-
"consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState),
170-
"height",
171-
newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision).encode());
136+
return handleConflict(clientId, tmHeader, clientState);
172137
}
173-
174138
// update the consensus state from a new header and set processed time metadata
175-
if (tmHeader.getSignedHeader().getHeader().getHeight().compareTo(clientState.getLatestHeight().getRevisionHeight()) > 0) {
176-
BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId());
177-
clientState.setLatestHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision));
178-
encodedClientState = clientState.encode();
179-
clientStates.set(clientId, encodedClientState);
180-
}
139+
return updateHeader(clientId, tmHeader, clientState, encodedClientState);
181140

182-
byte[] encodedConsensusState = toConsensusState(tmHeader).encode();
183-
consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
184-
encodedConsensusState);
185-
processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
186-
BigInteger.valueOf(Context.getBlockHeight()));
187-
processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
188-
BigInteger.valueOf(Context.getBlockTimestamp()));
189-
190-
return Map.of(
191-
"clientStateCommitment", IBCCommitment.keccak256(encodedClientState),
192-
"consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState),
193-
"height", clientState.getLatestHeight().encode());
194141
}
195142

196143
@External(readonly = true)
@@ -242,12 +189,79 @@ public void verifyNonMembership(
242189
Merkle.verifyNonMembership(merkleProof, Merkle.SDK_SPEC, root, merklePath);
243190
}
244191

192+
private Map<String, byte[]> updateHeader(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader,
193+
ClientState clientState, byte[] encodedClientState) {
194+
if (tmHeader.getSignedHeader().getHeader().getHeight()
195+
.compareTo(clientState.getLatestHeight().getRevisionHeight()) > 0) {
196+
BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId());
197+
clientState.setLatestHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision));
198+
encodedClientState = clientState.encode();
199+
clientStates.set(clientId, encodedClientState);
200+
}
201+
202+
byte[] encodedConsensusState = toConsensusState(tmHeader).encode();
203+
consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
204+
encodedConsensusState);
205+
processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
206+
BigInteger.valueOf(Context.getBlockHeight()));
207+
processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
208+
BigInteger.valueOf(Context.getBlockTimestamp()));
209+
210+
return Map.of(
211+
"clientStateCommitment", IBCCommitment.keccak256(encodedClientState),
212+
"consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState),
213+
"height", clientState.getLatestHeight().encode());
214+
}
215+
216+
private Map<String, byte[]> handleConflict(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader,
217+
ClientState clientState) {
218+
BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId());
219+
clientState.setFrozenHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision));
220+
byte[] encodedClientState = clientState.encode();
221+
clientStates.set(clientId, encodedClientState);
222+
223+
byte[] encodedConsensusState = toConsensusState(tmHeader).encode();
224+
consensusStates.at(clientId).set(clientState.getLatestHeight().getRevisionHeight(), encodedConsensusState);
225+
processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
226+
BigInteger.valueOf(Context.getBlockHeight()));
227+
processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(),
228+
BigInteger.valueOf(Context.getBlockTimestamp()));
229+
230+
return Map.of(
231+
"clientStateCommitment", IBCCommitment.keccak256(encodedClientState),
232+
"consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState),
233+
"height",
234+
newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision).encode());
235+
}
236+
237+
private boolean checkForDuplicateHeader(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader) {
238+
// Check if the Client store already has a consensus state for the header's
239+
// height
240+
// If the consensus state exists, and it matches the header then we return early
241+
// since header has already been submitted in a previous UpdateClient.
242+
byte[] prevConsState = consensusStates.at(clientId)
243+
.get(tmHeader.getSignedHeader().getHeader().getHeight());
244+
if (prevConsState == null) {
245+
return false;
246+
}
247+
248+
// This header has already been submitted and the necessary state is already
249+
// stored
250+
Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()),
251+
"LC: This header has already been submitted");
252+
253+
// A consensus state already exists for this height, but it does not match the
254+
// provided header.
255+
// Thus, we must check that this header is valid, and if so we will freeze the
256+
// client.
257+
return true;
258+
};
259+
245260
// checkValidity checks if the Tendermint header is valid.
246261
public void checkValidity(
247262
ClientState clientState,
248263
ConsensusState trustedConsensusState,
249-
ibc.lightclients.tendermint.v1.Header tmHeader,
250-
Timestamp currentTime) {
264+
ibc.lightclients.tendermint.v1.Header tmHeader) {
251265
// assert header height is newer than consensus state
252266
require(
253267
tmHeader.getSignedHeader().getHeader().getHeight()
@@ -267,11 +281,11 @@ public void checkValidity(
267281
SignedHeader untrustedHeader = tmHeader.getSignedHeader();
268282
ValidatorSet untrustedVals = tmHeader.getValidatorSet();
269283

284+
Timestamp currentTime = getCurrentTime();
270285
Context.require(!isExpired(trustedHeader, clientState.getTrustingPeriod(), currentTime),
271286
"header can't be expired");
272287

273288
boolean ok = verify(
274-
clientState.getTrustingPeriod(),
275289
clientState.getMaxClockDrift(),
276290
clientState.getTrustLevel(),
277291
trustedHeader,
@@ -287,15 +301,15 @@ private void validateArgs(ClientState cs, BigInteger height, byte[] prefix, byte
287301
Context.require(cs.getLatestHeight().getRevisionHeight().compareTo(height) >= 0,
288302
"Latest height must be greater or equal to proof height");
289303
Context.require(cs.getFrozenHeight().getRevisionHeight().equals(BigInteger.ZERO) ||
290-
cs.getFrozenHeight().getRevisionHeight().compareTo(height) >= 0,
304+
cs.getFrozenHeight().getRevisionHeight().compareTo(height) >= 0,
291305
"Client is Frozen");
292306
Context.require(prefix.length > 0, "Prefix cant be empty");
293307
Context.require(proof.length > 0, "Proof cant be empty");
294308
}
295309

296310
private void validateDelayPeriod(String clientId, Height height,
297-
BigInteger delayPeriodTime,
298-
BigInteger delayPeriodBlocks) {
311+
BigInteger delayPeriodTime,
312+
BigInteger delayPeriodBlocks) {
299313
BigInteger currentTime = BigInteger.valueOf(Context.getBlockTimestamp());
300314
BigInteger validTime = mustGetProcessedTime(clientId,
301315
height.getRevisionHeight()).add(delayPeriodTime);

‎contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java

+2-13
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313

1414
public abstract class Tendermint {
1515
protected boolean verify(
16-
Duration trustingPeriod,
1716
Duration maxClockDrift,
1817
Fraction trustLevel,
1918
SignedHeader trustedHeader,
@@ -26,29 +25,22 @@ protected boolean verify(
2625
boolean isAdjacent = untrustedHeader.getHeader().getHeight()
2726
.equals(trustedHeader.getHeader().getHeight().add(BigInteger.ONE));
2827
if (isAdjacent) {
29-
return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals, trustingPeriod, currentTime,
30-
maxClockDrift);
28+
return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals);
3129
}
3230

3331
return verifyNonAdjacent(
3432
trustedHeader,
3533
trustedVals,
3634
untrustedHeader,
3735
untrustedVals,
38-
trustingPeriod,
39-
currentTime,
40-
maxClockDrift,
4136
trustLevel);
4237

4338
}
4439

4540
protected boolean verifyAdjacent(
4641
SignedHeader trustedHeader,
4742
SignedHeader untrustedHeader,
48-
ValidatorSet untrustedVals,
49-
Duration trustingPeriod,
50-
Timestamp currentTime,
51-
Duration maxClockDrift) {
43+
ValidatorSet untrustedVals) {
5244

5345
// Check the validator hashes are the same
5446
Context.require(
@@ -71,9 +63,6 @@ protected boolean verifyNonAdjacent(
7163
ValidatorSet trustedVals,
7264
SignedHeader untrustedHeader,
7365
ValidatorSet untrustedVals,
74-
Duration trustingPeriod,
75-
Timestamp currentTime,
76-
Duration maxClockDrift,
7766
Fraction trustLevel) {
7867

7968
// assert that trustedVals is NextValidators of last trusted header

‎contracts/javascore/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,7 @@ void updateConflictingHeader() throws Exception {
160160
doNothing().when(clientSpy).checkValidity(
161161
any(ibc.lightclients.tendermint.v1.ClientState.class),
162162
any(ibc.lightclients.tendermint.v1.ConsensusState.class),
163-
any(ibc.lightclients.tendermint.v1.Header.class),
164-
any());
163+
any(ibc.lightclients.tendermint.v1.Header.class));
165164

166165
// Act
167166
updateClient(3, 1);

‎contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java

+2-13
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111

1212
public abstract class Tendermint {
1313
protected boolean verify(
14-
Duration trustingPeriod,
1514
Duration maxClockDrift,
1615
Fraction trustLevel,
1716
SignedHeader trustedHeader,
@@ -24,29 +23,22 @@ protected boolean verify(
2423
boolean isAdjacent = untrustedHeader.getHeader().getHeight()
2524
.equals(trustedHeader.getHeader().getHeight().add(BigInteger.ONE));
2625
if (isAdjacent) {
27-
return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals, trustingPeriod, currentTime,
28-
maxClockDrift);
26+
return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals);
2927
}
3028

3129
return verifyNonAdjacent(
3230
trustedHeader,
3331
trustedVals,
3432
untrustedHeader,
3533
untrustedVals,
36-
trustingPeriod,
37-
currentTime,
38-
maxClockDrift,
3934
trustLevel);
4035

4136
}
4237

4338
protected boolean verifyAdjacent(
4439
SignedHeader trustedHeader,
4540
SignedHeader untrustedHeader,
46-
ValidatorSet untrustedVals,
47-
Duration trustingPeriod,
48-
Timestamp currentTime,
49-
Duration maxClockDrift) {
41+
ValidatorSet untrustedVals) {
5042

5143
// Check the validator hashes are the same
5244
Context.require(
@@ -69,9 +61,6 @@ protected boolean verifyNonAdjacent(
6961
ValidatorSet trustedVals,
7062
SignedHeader untrustedHeader,
7163
ValidatorSet untrustedVals,
72-
Duration trustingPeriod,
73-
Timestamp currentTime,
74-
Duration maxClockDrift,
7564
Fraction trustLevel) {
7665

7766
// assert that trustedVals is NextValidators of last trusted header

0 commit comments

Comments
 (0)