fix: Require tendermint clientstate trustlevel to be >= 1/3 (#828)

* feat: Add revision number to light client heights

Add revision number but for now do not add support for resseting chain height

* wip

* revert to using two clients

* feat: add IBC prefix for ics-08 clients and bugfixes

* fix: Require tendermint clientstate trustlevel to be >= 1/3

* feat: Apply code improvments suggested during audit (#831)

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(),

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<String, byte[]> 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<String, byte[]> createClient(String clientId, byte[] clientStateBytes
     @External
     public Map<String, byte[]> 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<String, byte[]> 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<String, byte[]> 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);

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,29 +25,22 @@ 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(
                 trustedHeader,
                 trustedVals,
                 untrustedHeader,
                 untrustedVals,
-                trustingPeriod,
-                currentTime,
-                maxClockDrift,
                 trustLevel);
 
     }
 
     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

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]"
+        );
+    }
 }

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);