diff --git a/contracts/AVRValidator.sol b/contracts/AVRValidator.sol index 73d2ec2..b16857b 100644 --- a/contracts/AVRValidator.sol +++ b/contracts/AVRValidator.sol @@ -200,6 +200,10 @@ library AVRValidator { // find 'advisoryIDs' key and validate them checkpoint = consumeAdvisoryIdsReportJSON(report, checkpoint); validateAdvisories(report, checkpoint, allowedStatus.allowedAdvisories); + } else { + // NO-OP + // Since other statuses do not have `advisoryIDs` field, skip the validation here + // NOTE: In production, `allowedQuoteStatuses` should not allow these statuses } } @@ -259,13 +263,13 @@ library AVRValidator { } } else if (chr == CHAR_COMMA) { require( - allowedAdvisories[string(report[lastStart:lastStart + offset - lastStart - 1])] == FLAG_ALLOWED, + allowedAdvisories[string(report[lastStart:offset - 1])] == FLAG_ALLOWED, "disallowed advisory is included" ); } else if (chr == CHAR_LIST_END) { if (offset - lastStart > 0) { require( - allowedAdvisories[string(report[lastStart:lastStart + offset - lastStart - 1])] == FLAG_ALLOWED, + allowedAdvisories[string(report[lastStart:offset - 1])] == FLAG_ALLOWED, "disallowed advisory is included" ); } diff --git a/contracts/Asn1Decode.sol b/contracts/Asn1Decode.sol index c30c79b..97ca1ed 100644 --- a/contracts/Asn1Decode.sol +++ b/contracts/Asn1Decode.sol @@ -29,23 +29,26 @@ pragma solidity ^0.8.12; import "@ensdomains/ens-contracts/contracts/dnssec-oracle/BytesUtils.sol"; library NodePtr { - // Unpack first byte index + /// @dev Unpack first byte index function ixs(uint256 self) internal pure returns (uint256) { return uint80(self); } - // Unpack first content byte index + /// @dev Unpack first content byte index function ixf(uint256 self) internal pure returns (uint256) { return uint80(self >> 80); } - // Unpack last content byte index + /// @dev Unpack last content byte index function ixl(uint256 self) internal pure returns (uint256) { return uint80(self >> 160); } - // Pack 3 uint80s into a uint256 + /// @dev Pack 3 uint80s into a uint256 function getPtr(uint256 _ixs, uint256 _ixf, uint256 _ixl) internal pure returns (uint256) { + require(_ixs <= type(uint80).max); + require(_ixf <= type(uint80).max); + require(_ixl <= type(uint80).max); _ixs |= _ixf << 80; _ixs |= _ixl << 160; return _ixs; @@ -200,14 +203,16 @@ library Asn1Decode { function readNodeLength(bytes memory der, uint256 ix) private pure returns (uint256) { uint256 length; - uint80 ixFirstContentByte; - uint80 ixLastContentByte; - if ((der[ix + 1] & 0x80) == 0) { - length = uint8(der[ix + 1]); - ixFirstContentByte = uint80(ix + 2); - ixLastContentByte = uint80(ixFirstContentByte + length - 1); + uint256 ixFirstContentByte; + uint256 ixLastContentByte; + + uint8 b = uint8(der[ix + 1]); + if ((b & 0x80) == 0) { + length = b; + ixFirstContentByte = ix + 2; + ixLastContentByte = ixFirstContentByte + length - 1; } else { - uint8 lengthbytesLength = uint8(der[ix + 1] & 0x7F); + uint256 lengthbytesLength = uint256(b & 0x7F); if (lengthbytesLength == 1) { length = der.readUint8(ix + 2); } else if (lengthbytesLength == 2) { @@ -215,8 +220,8 @@ library Asn1Decode { } else { length = uint256(der.readBytesN(ix + 2, lengthbytesLength) >> (32 - lengthbytesLength) * 8); } - ixFirstContentByte = uint80(ix + 2 + lengthbytesLength); - ixLastContentByte = uint80(ixFirstContentByte + length - 1); + ixFirstContentByte = ix + 2 + lengthbytesLength; + ixLastContentByte = ixFirstContentByte + length - 1; } return NodePtr.getPtr(ix, ixFirstContentByte, ixLastContentByte); } diff --git a/contracts/ILCPClientErrors.sol b/contracts/ILCPClientErrors.sol index 7ef8fee..ab98229 100644 --- a/contracts/ILCPClientErrors.sol +++ b/contracts/ILCPClientErrors.sol @@ -34,6 +34,7 @@ interface ILCPClientErrors { error LCPClientUpdateStateEmittedStatesMustNotEmpty(); error LCPClientUpdateStatePrevStateIdMustNotEmpty(); error LCPClientUpdateStateUnexpectedPrevStateId(); + error LCPClientUpdateStateInconsistentConsensusState(); error LCPClientMisbehaviourPrevStatesMustNotEmpty(); diff --git a/contracts/LCPClientBase.sol b/contracts/LCPClientBase.sol index a0d707c..e9e7dd8 100644 --- a/contracts/LCPClientBase.sol +++ b/contracts/LCPClientBase.sol @@ -100,7 +100,8 @@ abstract contract LCPClientBase is ILightClient, ILCPClientErrors { /** * @dev initializeClient initializes a new client with the given state. * If succeeded, it returns heights at which the consensus state are stored. - * The function must be only called by IBCHandler. + * This function is guaranteed by the IBC contract to be called only once for each `clientId`. + * @param clientId the client identifier which is unique within the IBC handler */ function initializeClient( string calldata clientId, @@ -433,16 +434,22 @@ abstract contract LCPClientBase is ILightClient, ILCPClientErrors { LCPCommitment.validationContextEval(pmsg.context, block.timestamp * 1e9); - uint128 latestHeight = clientState.latest_height.toUint128(); uint128 postHeight = pmsg.postHeight.toUint128(); - if (latestHeight < postHeight) { - clientState.latest_height = pmsg.postHeight; - } - consensusState = clientStorage.consensusStates[postHeight]; + if (consensusState.stateId != bytes32(0)) { + if (consensusState.stateId != pmsg.postStateId || consensusState.timestamp != uint64(pmsg.timestamp)) { + revert LCPClientUpdateStateInconsistentConsensusState(); + } + // return empty heights if the consensus state is already stored + return heights; + } consensusState.stateId = pmsg.postStateId; consensusState.timestamp = uint64(pmsg.timestamp); + uint128 latestHeight = clientState.latest_height.toUint128(); + if (latestHeight < postHeight) { + clientState.latest_height = pmsg.postHeight; + } heights = new Height.Data[](1); heights[0] = pmsg.postHeight; return heights; diff --git a/test/LCPClientTest.t.sol b/test/LCPClientTest.t.sol index 5c2cb4a..a0f6715 100644 --- a/test/LCPClientTest.t.sol +++ b/test/LCPClientTest.t.sol @@ -118,6 +118,7 @@ contract LCPClientTest is BasicTest { } TestData[] memory dataList = readTestDataList(commandStartNumber); + bool firstUpdate = true; for (uint256 i = 0; i < dataList.length; i++) { if (dataList[i].cmd == Command.UpdateClient) { UpdateClientMessage.Data memory message = createUpdateClientMessage(dataList[i].path); @@ -125,6 +126,18 @@ contract LCPClientTest is BasicTest { require(heights.length == 1, "heights length must be 1"); console.log("revision_height:"); console.log(heights[0].revision_height); + if (firstUpdate) { + firstUpdate = false; + } else { + // repeat updateClient to check the state is not changed + message = createUpdateClientMessage(dataList[i].path); + // staticcall is expected to succeed because updateClient does not update the state if the message is already processed + (bool success, bytes memory ret) = address(lc).staticcall( + abi.encodeWithSelector(LCPClientBase.updateClient.selector, clientId, message) + ); + heights = abi.decode(ret, (Height.Data[])); + require(heights.length == 0, "heights length must be 0"); + } } else if (dataList[i].cmd == Command.VerifyMembership) { ( Height.Data memory height,