Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Apply code improvments suggested during audit #831

Merged
merged 1 commit into from
Mar 21, 2024
Merged

feat: Apply code improvments suggested during audit #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion contracts/javascore/ibc/src/main/java/ibc/ics04/channel/IBCPacket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,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(),
Expand Down
156 changes: 85 additions & 71 deletions ...ents/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/ICS08TendermintLightClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
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;
import ibc.ics24.host.IBCCommitment;
import cosmos.ics23.v1.*;
import google.protobuf.*;
import tendermint.types.*;
import ibc.core.commitment.v1.*;
Expand All @@ -17,7 +15,6 @@
import score.Context;
import score.DictDB;
import score.annotation.External;
import score.annotation.Optional;

import java.math.BigInteger;
import java.util.Arrays;
Expand Down Expand Up @@ -54,7 +51,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(
Expand Down Expand Up @@ -118,79 +115,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)
Expand Down Expand Up @@ -242,12 +189,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()
Expand All @@ -267,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,
Expand All @@ -287,15 +301,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);
Expand Down
15 changes: 2 additions & 13 deletions ...vascore/lightclients/ics-08-tendermint/src/main/java/ibc/ics08/tendermint/Tendermint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

public abstract class Tendermint {
protected boolean verify(
Duration trustingPeriod,
Duration maxClockDrift,
Fraction trustLevel,
SignedHeader trustedHeader,
Expand All @@ -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(
Expand All @@ -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
Expand Down
3 changes: 1 addition & 2 deletions ...re/lightclients/ics-08-tendermint/src/test/java/ibc/ics08/tendermint/LightClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,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);
Expand Down
15 changes: 2 additions & 13 deletions contracts/javascore/lightclients/tendermint/src/main/java/ibc/tendermint/Tendermint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

public abstract class Tendermint {
protected boolean verify(
Duration trustingPeriod,
Duration maxClockDrift,
Fraction trustLevel,
SignedHeader trustedHeader,
Expand All @@ -24,29 +23,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(
Expand All @@ -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
Expand Down
Loading
Loading