diff --git a/contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java b/contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java index 08812678e..2a134411d 100644 --- a/contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java +++ b/contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java @@ -291,11 +291,15 @@ public void _timeoutPacket(Packet packet, byte[] proofHeight, byte[] proof, BigI ConnectionEnd connection = ConnectionEnd.decode(connectionPb); // check that timeout height or timeout timestamp has passed on the other end + ILightClient client = getClient(connection.getClientId()); Height height = Height.decode(proofHeight); + BigInteger timestamp = client.getTimestampAtHeight(connection.getClientId(), proofHeight); boolean heightTimeout = packet.getTimeoutHeight().getRevisionHeight().compareTo(BigInteger.ZERO) > 0 && height.getRevisionHeight() .compareTo(packet.getTimeoutHeight().getRevisionHeight()) >= 0; - Context.require(heightTimeout, "Packet has not yet timed out"); + boolean timeTimeout = packet.getTimeoutTimestamp().compareTo(BigInteger.ZERO) > 0 + && timestamp.compareTo(packet.getTimeoutTimestamp()) >= 0; + Context.require(heightTimeout || timeTimeout, "Packet has not yet timed out"); // verify we actually sent this packet, check the store byte[] packetCommitmentKey = IBCCommitment.packetCommitmentKey(packet.getSourcePort(), diff --git a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java index 4e9b4664c..beb28580f 100644 --- a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java +++ b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java @@ -1,7 +1,6 @@ package ibc.ics08.tendermint; import icon.ibc.interfaces.ILightClient; -import ibc.icon.score.util.ByteUtil; import ibc.icon.score.util.NullChecker; import ibc.icon.score.util.StringUtil; import ibc.ics23.commitment.types.Merkle; @@ -17,7 +16,6 @@ import score.Context; import score.DictDB; import score.annotation.External; -import score.annotation.Optional; import java.math.BigInteger; import java.util.Arrays; @@ -54,7 +52,7 @@ private void onlyHandler() { /** * @dev getTimestampAtHeight returns the timestamp of the consensus state at the - * given height. + * given height. */ @External(readonly = true) public BigInteger getTimestampAtHeight( @@ -101,8 +99,7 @@ public Map createClient(String clientId, byte[] clientStateBytes Context.require(clientStates.get(clientId) == null, "Client already exists"); ClientState clientState = ClientState.decode(clientStateBytes); - Context.require(!clientState.getTrustLevel().getDenominator().equals(BigInteger.ZERO), - "trustLevel has zero Denominator"); + validateTrustLevel(clientState.getTrustLevel()); clientStates.set(clientId, clientStateBytes); consensusStates.at(clientId).set(clientState.getLatestHeight().getRevisionHeight(), consensusStateBytes); @@ -119,79 +116,29 @@ public Map createClient(String clientId, byte[] clientStateBytes @External public Map updateClient(String clientId, byte[] clientMessageBytes) { onlyHandler(); - ibc.lightclients.tendermint.v1.Header tmHeader = ibc.lightclients.tendermint.v1.Header.decode(clientMessageBytes); - boolean conflictingHeader = false; - // Check if the Client store already has a consensus state for the header's - // height - // If the consensus state exists, and it matches the header then we return early - // since header has already been submitted in a previous UpdateClient. - byte[] prevConsState = consensusStates.at(clientId) - .get(tmHeader.getSignedHeader().getHeader().getHeight()); - if (prevConsState != null) { - // This header has already been submitted and the necessary state is already - // stored - Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()), - "LC: This header has already been submitted"); - - // A consensus state already exists for this height, but it does not match the - // provided header. - // Thus, we must check that this header is valid, and if so we will freeze the - // client. - conflictingHeader = true; - } + ibc.lightclients.tendermint.v1.Header tmHeader = ibc.lightclients.tendermint.v1.Header + .decode(clientMessageBytes); + boolean conflictingHeader = checkForDuplicateHeader(clientId, tmHeader); byte[] encodedClientState = clientStates.get(clientId); require(encodedClientState != null, "LC: client state is invalid"); ClientState clientState = ClientState.decode(encodedClientState); - byte[] encodedTrustedonsensusState = consensusStates.at(clientId).get(tmHeader.getTrustedHeight().getRevisionHeight()); - require(encodedTrustedonsensusState != null, "LC: consensusState not found at trusted height"); - ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedonsensusState); - Timestamp currentTime = getCurrentTime(); - checkValidity(clientState, trustedConsensusState, tmHeader, currentTime); + byte[] encodedTrustedConsensusState = consensusStates.at(clientId) + .get(tmHeader.getTrustedHeight().getRevisionHeight()); + require(encodedTrustedConsensusState != null, "LC: consensusState not found at trusted height"); + ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedConsensusState); + + checkValidity(clientState, trustedConsensusState, tmHeader); // Header is different from existing consensus state and also valid, so freeze // the client and return if (conflictingHeader) { - BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId()); - clientState.setFrozenHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision)); - encodedClientState = clientState.encode(); - clientStates.set(clientId, encodedClientState); - - byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); - consensusStates.at(clientId).set(clientState.getLatestHeight().getRevisionHeight(), encodedConsensusState); - processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockHeight())); - processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockTimestamp())); - - return Map.of( - "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), - "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), - "height", - newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision).encode()); + return handleConflict(clientId, tmHeader, clientState); } - // update the consensus state from a new header and set processed time metadata - if (tmHeader.getSignedHeader().getHeader().getHeight().compareTo(clientState.getLatestHeight().getRevisionHeight()) > 0) { - BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId()); - clientState.setLatestHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision)); - encodedClientState = clientState.encode(); - clientStates.set(clientId, encodedClientState); - } + return updateHeader(clientId, tmHeader, clientState, encodedClientState); - byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); - consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - encodedConsensusState); - processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockHeight())); - processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockTimestamp())); - - return Map.of( - "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), - "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), - "height", clientState.getLatestHeight().encode()); } @External(readonly = true) @@ -243,12 +190,79 @@ public void verifyNonMembership( Merkle.verifyNonMembership(merkleProof, Merkle.SDK_SPEC, root, merklePath); } + private Map updateHeader(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader, + ClientState clientState, byte[] encodedClientState) { + if (tmHeader.getSignedHeader().getHeader().getHeight() + .compareTo(clientState.getLatestHeight().getRevisionHeight()) > 0) { + BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId()); + clientState.setLatestHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision)); + encodedClientState = clientState.encode(); + clientStates.set(clientId, encodedClientState); + } + + byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); + consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + encodedConsensusState); + processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockHeight())); + processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockTimestamp())); + + return Map.of( + "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), + "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), + "height", clientState.getLatestHeight().encode()); + } + + private Map handleConflict(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader, + ClientState clientState) { + BigInteger revision = getRevisionNumber(tmHeader.getSignedHeader().getHeader().getChainId()); + clientState.setFrozenHeight(newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision)); + byte[] encodedClientState = clientState.encode(); + clientStates.set(clientId, encodedClientState); + + byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); + consensusStates.at(clientId).set(clientState.getLatestHeight().getRevisionHeight(), encodedConsensusState); + processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockHeight())); + processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockTimestamp())); + + return Map.of( + "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), + "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), + "height", + newHeight(tmHeader.getSignedHeader().getHeader().getHeight(), revision).encode()); + } + + private boolean checkForDuplicateHeader(String clientId, ibc.lightclients.tendermint.v1.Header tmHeader) { + // Check if the Client store already has a consensus state for the header's + // height + // If the consensus state exists, and it matches the header then we return early + // since header has already been submitted in a previous UpdateClient. + byte[] prevConsState = consensusStates.at(clientId) + .get(tmHeader.getSignedHeader().getHeader().getHeight()); + if (prevConsState == null) { + return false; + } + + // This header has already been submitted and the necessary state is already + // stored + Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()), + "LC: This header has already been submitted"); + + // A consensus state already exists for this height, but it does not match the + // provided header. + // Thus, we must check that this header is valid, and if so we will freeze the + // client. + return true; + }; + // checkValidity checks if the Tendermint header is valid. public void checkValidity( ClientState clientState, ConsensusState trustedConsensusState, - ibc.lightclients.tendermint.v1.Header tmHeader, - Timestamp currentTime) { + ibc.lightclients.tendermint.v1.Header tmHeader) { // assert header height is newer than consensus state require( tmHeader.getSignedHeader().getHeader().getHeight() @@ -268,11 +282,11 @@ public void checkValidity( SignedHeader untrustedHeader = tmHeader.getSignedHeader(); ValidatorSet untrustedVals = tmHeader.getValidatorSet(); + Timestamp currentTime = getCurrentTime(); Context.require(!isExpired(trustedHeader, clientState.getTrustingPeriod(), currentTime), "header can't be expired"); boolean ok = verify( - clientState.getTrustingPeriod(), clientState.getMaxClockDrift(), clientState.getTrustLevel(), trustedHeader, @@ -288,15 +302,15 @@ private void validateArgs(ClientState cs, BigInteger height, byte[] prefix, byte Context.require(cs.getLatestHeight().getRevisionHeight().compareTo(height) >= 0, "Latest height must be greater or equal to proof height"); Context.require(cs.getFrozenHeight().getRevisionHeight().equals(BigInteger.ZERO) || - cs.getFrozenHeight().getRevisionHeight().compareTo(height) >= 0, + cs.getFrozenHeight().getRevisionHeight().compareTo(height) >= 0, "Client is Frozen"); Context.require(prefix.length > 0, "Prefix cant be empty"); Context.require(proof.length > 0, "Proof cant be empty"); } private void validateDelayPeriod(String clientId, Height height, - BigInteger delayPeriodTime, - BigInteger delayPeriodBlocks) { + BigInteger delayPeriodTime, + BigInteger delayPeriodBlocks) { BigInteger currentTime = BigInteger.valueOf(Context.getBlockTimestamp()); BigInteger validTime = mustGetProcessedTime(clientId, height.getRevisionHeight()).add(delayPeriodTime); diff --git a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java index 70b35251f..ba921fcf4 100644 --- a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java +++ b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java @@ -13,7 +13,6 @@ public abstract class Tendermint { protected boolean verify( - Duration trustingPeriod, Duration maxClockDrift, Fraction trustLevel, SignedHeader trustedHeader, @@ -26,8 +25,7 @@ protected boolean verify( boolean isAdjacent = untrustedHeader.getHeader().getHeight() .equals(trustedHeader.getHeader().getHeight().add(BigInteger.ONE)); if (isAdjacent) { - return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals, trustingPeriod, currentTime, - maxClockDrift); + return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals); } return verifyNonAdjacent( @@ -35,9 +33,6 @@ protected boolean verify( trustedVals, untrustedHeader, untrustedVals, - trustingPeriod, - currentTime, - maxClockDrift, trustLevel); } @@ -45,10 +40,7 @@ protected boolean verify( protected boolean verifyAdjacent( SignedHeader trustedHeader, SignedHeader untrustedHeader, - ValidatorSet untrustedVals, - Duration trustingPeriod, - Timestamp currentTime, - Duration maxClockDrift) { + ValidatorSet untrustedVals) { // Check the validator hashes are the same Context.require( @@ -71,9 +63,6 @@ protected boolean verifyNonAdjacent( ValidatorSet trustedVals, SignedHeader untrustedHeader, ValidatorSet untrustedVals, - Duration trustingPeriod, - Timestamp currentTime, - Duration maxClockDrift, Fraction trustLevel) { // assert that trustedVals is NextValidators of last trusted header diff --git a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/TendermintHelper.java b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/TendermintHelper.java index ce8c4a809..74bc8a84d 100644 --- a/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/TendermintHelper.java +++ b/contracts/javascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/TendermintHelper.java @@ -9,6 +9,7 @@ import tendermint.types.Commit; import tendermint.types.CommitSig; import ibc.lightclients.tendermint.v1.ConsensusState; +import ibc.lightclients.tendermint.v1.Fraction; import tendermint.types.Header; import ibc.core.commitment.v1.MerkleRoot; import tendermint.types.SignedHeader; @@ -159,4 +160,13 @@ public static byte[] hash(Header header) { return MerkleTree.merkleRootHash(all, 0, all.length); } + + public static void validateTrustLevel(Fraction tl) { + Context.require( + tl.getNumerator().multiply(BigInteger.valueOf(3)).compareTo(tl.getDenominator()) >= 0 && + tl.getNumerator().compareTo(tl.getDenominator()) <= 0 && + !tl.getDenominator().equals(BigInteger.ZERO), + "trustLevel must be within [1/3, 1]" + ); + } } diff --git a/contracts/javascore/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java b/contracts/javascore/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java index 0f934fa91..a97065eeb 100644 --- a/contracts/javascore/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java +++ b/contracts/javascore/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java @@ -52,15 +52,28 @@ void createClient() throws Exception { } @Test - void createClient_withZeroDenomTrustLevel() throws Exception { + void createClient_trustLevelRestriction() throws Exception { // Arrange - // Default is zero denominator - trustLevel = Fraction.getDefaultInstance(); - String expectedErrorMessage = "trustLevel has zero Denominator"; + String expectedErrorMessage = "trustLevel must be within [1/3, 1]"; // Act & Assert + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(1) + .setDenominator(0).build(); AssertionError e = assertThrows(AssertionError.class, () -> initializeClient(1)); assertTrue(e.getMessage().contains(expectedErrorMessage)); + + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(2) + .setDenominator(1).build(); + e = assertThrows(AssertionError.class, () -> initializeClient(1)); + assertTrue(e.getMessage().contains(expectedErrorMessage)); + + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(1) + .setDenominator(4).build(); + e = assertThrows(AssertionError.class, () -> initializeClient(1)); + assertTrue(e.getMessage().contains(expectedErrorMessage)); } @Test @@ -147,8 +160,7 @@ void updateConflictingHeader() throws Exception { doNothing().when(clientSpy).checkValidity( any(ibc.lightclients.tendermint.v1.ClientState.class), any(ibc.lightclients.tendermint.v1.ConsensusState.class), - any(ibc.lightclients.tendermint.v1.Header.class), - any()); + any(ibc.lightclients.tendermint.v1.Header.class)); // Act updateClient(3, 1); diff --git a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java index 11f3aa8ff..531d64bf8 100644 --- a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java +++ b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java @@ -11,7 +11,6 @@ public abstract class Tendermint { protected boolean verify( - Duration trustingPeriod, Duration maxClockDrift, Fraction trustLevel, SignedHeader trustedHeader, @@ -24,8 +23,7 @@ protected boolean verify( boolean isAdjacent = untrustedHeader.getHeader().getHeight() .equals(trustedHeader.getHeader().getHeight().add(BigInteger.ONE)); if (isAdjacent) { - return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals, trustingPeriod, currentTime, - maxClockDrift); + return verifyAdjacent(trustedHeader, untrustedHeader, untrustedVals); } return verifyNonAdjacent( @@ -33,9 +31,6 @@ protected boolean verify( trustedVals, untrustedHeader, untrustedVals, - trustingPeriod, - currentTime, - maxClockDrift, trustLevel); } @@ -43,10 +38,7 @@ protected boolean verify( protected boolean verifyAdjacent( SignedHeader trustedHeader, SignedHeader untrustedHeader, - ValidatorSet untrustedVals, - Duration trustingPeriod, - Timestamp currentTime, - Duration maxClockDrift) { + ValidatorSet untrustedVals) { // Check the validator hashes are the same Context.require( @@ -69,9 +61,6 @@ protected boolean verifyNonAdjacent( ValidatorSet trustedVals, SignedHeader untrustedHeader, ValidatorSet untrustedVals, - Duration trustingPeriod, - Timestamp currentTime, - Duration maxClockDrift, Fraction trustLevel) { // assert that trustedVals is NextValidators of last trusted header diff --git a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintHelper.java b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintHelper.java index a0ea42cfc..0b88e8127 100644 --- a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintHelper.java +++ b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintHelper.java @@ -18,6 +18,7 @@ import icon.proto.clients.tendermint.TmHeader; import icon.proto.clients.tendermint.Validator; import icon.proto.clients.tendermint.ValidatorSet; +import icon.proto.clients.tendermint.Fraction; import ibc.core.commitment.v1.MerkleRoot; import icon.proto.core.client.Height; import score.Context; @@ -153,4 +154,13 @@ public static byte[] hash(LightHeader header) { return MerkleTree.merkleRootHash(all, 0, all.length); } + + public static void validateTrustLevel(Fraction tl) { + Context.require( + tl.getNumerator().multiply(BigInteger.valueOf(3)).compareTo(tl.getDenominator()) >= 0 && + tl.getNumerator().compareTo(tl.getDenominator()) <= 0 && + !tl.getDenominator().equals(BigInteger.ZERO), + "trustLevel must be within [1/3, 1]" + ); + } } diff --git a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintLightClient.java b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintLightClient.java index aff686b6f..062761560 100644 --- a/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintLightClient.java +++ b/contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/TendermintLightClient.java @@ -97,8 +97,7 @@ public Map createClient(String clientId, byte[] clientStateBytes Context.require(clientStates.get(clientId) == null, "Client already exists"); ClientState clientState = ClientState.decode(clientStateBytes); - Context.require(!clientState.getTrustLevel().getDenominator().equals(BigInteger.ZERO), - "trustLevel has zero Denominator"); + validateTrustLevel(clientState.getTrustLevel()); clientStates.set(clientId, clientStateBytes); consensusStates.at(clientId).set(clientState.getLatestHeight(), consensusStateBytes); @@ -116,77 +115,26 @@ public Map createClient(String clientId, byte[] clientStateBytes public Map updateClient(String clientId, byte[] clientMessageBytes) { onlyHandler(); TmHeader tmHeader = TmHeader.decode(clientMessageBytes); - boolean conflictingHeader = false; - - // Check if the Client store already has a consensus state for the header's - // height - // If the consensus state exists, and it matches the header then we return early - // since header has already been submitted in a previous UpdateClient. - byte[] prevConsState = consensusStates.at(clientId) - .get(tmHeader.getSignedHeader().getHeader().getHeight()); - if (prevConsState != null) { - // This header has already been submitted and the necessary state is already - // stored - Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()), - "LC: This header has already been submitted"); - - // A consensus state already exists for this height, but it does not match the - // provided header. - // Thus, we must check that this header is valid, and if so we will freeze the - // client. - conflictingHeader = true; - } + boolean conflictingHeader = checkForDuplicateHeader(clientId, tmHeader); byte[] encodedClientState = clientStates.get(clientId); require(encodedClientState != null, "LC: client state is invalid"); ClientState clientState = ClientState.decode(encodedClientState); - byte[] encodedTrustedonsensusState = consensusStates.at(clientId).get(tmHeader.getTrustedHeight()); - require(encodedTrustedonsensusState != null, "LC: consensusState not found at trusted height"); - ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedonsensusState); - Timestamp currentTime = getCurrentTime(); - checkValidity(clientState, trustedConsensusState, tmHeader, currentTime); + byte[] encodedTrustedConsensusState = consensusStates.at(clientId).get(tmHeader.getTrustedHeight()); + require(encodedTrustedConsensusState != null, "LC: consensusState not found at trusted height"); + ConsensusState trustedConsensusState = ConsensusState.decode(encodedTrustedConsensusState); + + checkValidity(clientState, trustedConsensusState, tmHeader); // Header is different from existing consensus state and also valid, so freeze // the client and return if (conflictingHeader) { - clientState.setFrozenHeight(tmHeader.getSignedHeader().getHeader().getHeight()); - encodedClientState = clientState.encode(); - clientStates.set(clientId, encodedClientState); - - byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); - consensusStates.at(clientId).set(clientState.getLatestHeight(), encodedConsensusState); - processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockHeight())); - processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockTimestamp())); - - return Map.of( - "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), - "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), - "height", - newHeight(tmHeader.getSignedHeader().getHeader().getHeight()).encode()); + return handleConflict(clientId, tmHeader, clientState); } // update the consensus state from a new header and set processed time metadata - if (tmHeader.getSignedHeader().getHeader().getHeight().compareTo(clientState.getLatestHeight()) > 0) { - clientState.setLatestHeight(tmHeader.getSignedHeader().getHeader().getHeight()); - encodedClientState = clientState.encode(); - clientStates.set(clientId, encodedClientState); - } - - byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); - consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - encodedConsensusState); - processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockHeight())); - processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), - BigInteger.valueOf(Context.getBlockTimestamp())); - - return Map.of( - "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), - "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), - "height", newHeight(clientState.getLatestHeight()).encode()); + return updateHeader(clientId, tmHeader, clientState, encodedClientState); } @External(readonly = true) @@ -245,12 +193,75 @@ public void verifyNonMembership( Merkle.verifyNonMembership(merkleProof, Merkle.SDK_SPEC, root, merklePath); } + private Map updateHeader(String clientId, TmHeader tmHeader, ClientState clientState, + byte[] encodedClientState) { + if (tmHeader.getSignedHeader().getHeader().getHeight().compareTo(clientState.getLatestHeight()) > 0) { + clientState.setLatestHeight(tmHeader.getSignedHeader().getHeader().getHeight()); + encodedClientState = clientState.encode(); + clientStates.set(clientId, encodedClientState); + } + + byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); + consensusStates.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + encodedConsensusState); + processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockHeight())); + processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockTimestamp())); + + return Map.of( + "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), + "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), + "height", newHeight(clientState.getLatestHeight()).encode()); + } + + private Map handleConflict(String clientId, TmHeader tmHeader, ClientState clientState) { + clientState.setFrozenHeight(tmHeader.getSignedHeader().getHeader().getHeight()); + byte[] encodedClientState = clientState.encode(); + clientStates.set(clientId, encodedClientState); + + byte[] encodedConsensusState = toConsensusState(tmHeader).encode(); + consensusStates.at(clientId).set(clientState.getLatestHeight(), encodedConsensusState); + processedHeights.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockHeight())); + processedTimes.at(clientId).set(tmHeader.getSignedHeader().getHeader().getHeight(), + BigInteger.valueOf(Context.getBlockTimestamp())); + + return Map.of( + "clientStateCommitment", IBCCommitment.keccak256(encodedClientState), + "consensusStateCommitment", IBCCommitment.keccak256(encodedConsensusState), + "height", + newHeight(tmHeader.getSignedHeader().getHeader().getHeight()).encode()); + } + + private boolean checkForDuplicateHeader(String clientId, TmHeader tmHeader) { + // Check if the Client store already has a consensus state for the header's + // height + // If the consensus state exists, and it matches the header then we return early + // since header has already been submitted in a previous UpdateClient. + byte[] prevConsState = consensusStates.at(clientId) + .get(tmHeader.getSignedHeader().getHeader().getHeight()); + if (prevConsState == null) { + return false; + } + + // This header has already been submitted and the necessary state is already + // stored + Context.require(!Arrays.equals(prevConsState, toConsensusState(tmHeader).encode()), + "LC: This header has already been submitted"); + + // A consensus state already exists for this height, but it does not match the + // provided header. + // Thus, we must check that this header is valid, and if so we will freeze the + // client. + return true; + }; + // checkValidity checks if the Tendermint header is valid. public void checkValidity( ClientState clientState, ConsensusState trustedConsensusState, - TmHeader tmHeader, - Timestamp currentTime) { + TmHeader tmHeader) { // assert header height is newer than consensus state require( tmHeader.getSignedHeader().getHeader().getHeight() @@ -270,11 +281,11 @@ public void checkValidity( SignedHeader untrustedHeader = tmHeader.getSignedHeader(); ValidatorSet untrustedVals = tmHeader.getValidatorSet(); + Timestamp currentTime = getCurrentTime(); Context.require(!isExpired(trustedHeader, clientState.getTrustingPeriod(), currentTime), "header can't be expired"); boolean ok = verify( - clientState.getTrustingPeriod(), clientState.getMaxClockDrift(), clientState.getTrustLevel(), trustedHeader, @@ -290,15 +301,15 @@ private void validateArgs(ClientState cs, BigInteger height, byte[] prefix, byte Context.require(cs.getLatestHeight().compareTo(height) >= 0, "Latest height must be greater or equal to proof height"); Context.require(cs.getFrozenHeight().equals(BigInteger.ZERO) || - cs.getFrozenHeight().compareTo(height) >= 0, + cs.getFrozenHeight().compareTo(height) >= 0, "Client is Frozen"); Context.require(prefix.length > 0, "Prefix cant be empty"); Context.require(proof.length > 0, "Proof cant be empty"); } private void validateDelayPeriod(String clientId, Height height, - BigInteger delayPeriodTime, - BigInteger delayPeriodBlocks) { + BigInteger delayPeriodTime, + BigInteger delayPeriodBlocks) { BigInteger currentTime = BigInteger.valueOf(Context.getBlockTimestamp()); BigInteger validTime = mustGetProcessedTime(clientId, height.getRevisionHeight()).add(delayPeriodTime); diff --git a/contracts/javascore/lightclients/tendermint/src/test/java/ibc/tendermint/LightClientTest.java b/contracts/javascore/lightclients/tendermint/src/test/java/ibc/tendermint/LightClientTest.java index b7260667c..df5666ef9 100644 --- a/contracts/javascore/lightclients/tendermint/src/test/java/ibc/tendermint/LightClientTest.java +++ b/contracts/javascore/lightclients/tendermint/src/test/java/ibc/tendermint/LightClientTest.java @@ -51,15 +51,28 @@ void createClient() throws Exception { } @Test - void createClient_withZeroDenomTrustLevel() throws Exception { + void createClient_trustLevelRestriction() throws Exception { // Arrange - // Default is zero denominator - trustLevel = Fraction.getDefaultInstance(); - String expectedErrorMessage = "trustLevel has zero Denominator"; + String expectedErrorMessage = "trustLevel must be within [1/3, 1]"; // Act & Assert + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(1) + .setDenominator(0).build(); AssertionError e = assertThrows(AssertionError.class, () -> initializeClient(1)); assertTrue(e.getMessage().contains(expectedErrorMessage)); + + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(2) + .setDenominator(1).build(); + e = assertThrows(AssertionError.class, () -> initializeClient(1)); + assertTrue(e.getMessage().contains(expectedErrorMessage)); + + trustLevel = trustLevel = Fraction.newBuilder() + .setNumerator(1) + .setDenominator(4).build(); + e = assertThrows(AssertionError.class, () -> initializeClient(1)); + assertTrue(e.getMessage().contains(expectedErrorMessage)); } @Test @@ -165,8 +178,7 @@ void updateConflictingHeader() throws Exception { doNothing().when(clientSpy).checkValidity( any(icon.proto.clients.tendermint.ClientState.class), any(icon.proto.clients.tendermint.ConsensusState.class), - any(TmHeader.class), - any(Timestamp.class)); + any(TmHeader.class)); // Act updateClient(3, 1);