Merge pull request #25 from datachainlab/audit-202409-s2-s3

Fix S2 and S3

Signed-off-by: Jun Kimura <[email protected]>

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

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,23 +203,25 @@ 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) {
                 length = der.readUint16(ix + 2);
             } 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);
     }

contracts/ILCPClientErrors.sol

@@ -34,6 +34,7 @@ interface ILCPClientErrors {
     error LCPClientUpdateStateEmittedStatesMustNotEmpty();
     error LCPClientUpdateStatePrevStateIdMustNotEmpty();
     error LCPClientUpdateStateUnexpectedPrevStateId();
+    error LCPClientUpdateStateInconsistentConsensusState();
 
     error LCPClientMisbehaviourPrevStatesMustNotEmpty();
 

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;

test/LCPClientTest.t.sol

@@ -118,13 +118,26 @@ 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);
                 Height.Data[] memory heights = lc.updateClient(clientId, message);
                 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,