Merge pull request #24 from datachainlab/audit-202409

Audit-202409

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,29 +29,33 @@ 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;
     }
 }
 
+// slither-disable-start dead-code
 library Asn1Decode {
     using NodePtr for uint256;
     using BytesUtils for bytes;
@@ -200,24 +204,27 @@ 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);
     }
 }
+// slither-disable-end dead-code

contracts/ILCPClientErrors.sol

@@ -8,11 +8,12 @@ interface ILCPClientErrors {
     error LCPClientClientStateInvalidKeyExpiration();
     error LCPClientClientStateInvalidMrenclaveLength();
     error LCPClientClientStateUnexpectedMrenclave();
-    error LCPClientClientStateEmptyOperators();
     error LCPClientClientStateInvalidOperatorAddress();
     error LCPClientClientStateInvalidOperatorAddressLength();
     error LCPClientClientStateInvalidOperatorsNonce();
     error LCPClientClientStateUnexpectedOperatorsNonce(uint64 expectedNonce);
+    error LCPClientClientStateInvalidAllowedQuoteStatus();
+    error LCPClientClientStateInvalidAllowedAdvisoryId();
 
     error LCPClientOperatorsInvalidOrder(address prevOperator, address nextOperator);
     error LCPClientClientStateInvalidOperatorsThreshold();
@@ -34,6 +35,7 @@ interface ILCPClientErrors {
     error LCPClientUpdateStateEmittedStatesMustNotEmpty();
     error LCPClientUpdateStatePrevStateIdMustNotEmpty();
     error LCPClientUpdateStateUnexpectedPrevStateId();
+    error LCPClientUpdateStateInconsistentConsensusState();
 
     error LCPClientMisbehaviourPrevStatesMustNotEmpty();
 

contracts/LCPClientBase.sol

@@ -57,13 +57,17 @@ abstract contract LCPClientBase is ILightClient, ILCPClientErrors {
 
     // --------------------- Storage fields ---------------------
 
+    /// @dev clientId => client storage
     mapping(string => ClientStorage) internal clientStorages;
 
-    // rootCA's public key parameters
+    /// @dev RootCA's public key parameters
     AVRValidator.RSAParams internal verifiedRootCAParams;
-    // keccak256(signingCert) => RSAParams of signing public key
+    /// @dev keccak256(signingCert) => RSAParams of signing public key
     mapping(bytes32 => AVRValidator.RSAParams) internal verifiedSigningRSAParams;
 
+    /// @dev Reserved storage space to allow for layout changes in the future
+    uint256[50] private __gap;
+
     // --------------------- Constructor ---------------------
 
     /// @custom:oz-upgrades-unsafe-allow constructor
@@ -100,7 +104,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,
@@ -164,12 +169,18 @@ abstract contract LCPClientBase is ILightClient, ILCPClientErrors {
 
         // set allowed quote status and advisories
         for (uint256 i = 0; i < clientState.allowed_quote_statuses.length; i++) {
-            clientStorage.allowedStatuses.allowedQuoteStatuses[clientState.allowed_quote_statuses[i]] =
-                AVRValidator.FLAG_ALLOWED;
+            string memory allowedQuoteStatus = clientState.allowed_quote_statuses[i];
+            if (bytes(allowedQuoteStatus).length == 0) {
+                revert LCPClientClientStateInvalidAllowedQuoteStatus();
+            }
+            clientStorage.allowedStatuses.allowedQuoteStatuses[allowedQuoteStatus] = AVRValidator.FLAG_ALLOWED;
         }
         for (uint256 i = 0; i < clientState.allowed_advisory_ids.length; i++) {
-            clientStorage.allowedStatuses.allowedAdvisories[clientState.allowed_advisory_ids[i]] =
-                AVRValidator.FLAG_ALLOWED;
+            string memory allowedAdvisoryId = clientState.allowed_advisory_ids[i];
+            if (bytes(allowedAdvisoryId).length == 0) {
+                revert LCPClientClientStateInvalidAllowedAdvisoryId();
+            }
+            clientStorage.allowedStatuses.allowedAdvisories[allowedAdvisoryId] = AVRValidator.FLAG_ALLOWED;
         }
 
         return clientState.latest_height;
@@ -433,16 +444,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;
@@ -639,14 +656,6 @@ abstract contract LCPClientBase is ILightClient, ILCPClientErrors {
         }
     }
 
-    function verifyECDSASignature(bytes32 commitment, bytes memory signature, address signer)
-        internal
-        pure
-        returns (bool)
-    {
-        return verifyECDSASignature(commitment, signature) == signer;
-    }
-
     function verifyECDSASignature(bytes32 commitment, bytes memory signature) internal pure returns (address) {
         if (uint8(signature[64]) < 27) {
             signature[64] = bytes1(uint8(signature[64]) + 27);

contracts/LCPCommitment.sol

@@ -44,22 +44,6 @@ library LCPCommitment {
         bytes state;
     }
 
-    function parseUpdateStateProxyMessage(bytes calldata messageBytes)
-        internal
-        pure
-        returns (UpdateStateProxyMessage memory)
-    {
-        HeaderedProxyMessage memory hm = abi.decode(messageBytes, (HeaderedProxyMessage));
-        // MSB first
-        // 0-1:  version
-        // 2-3:  message type
-        // 4-31: reserved
-        if (hm.header != LCP_MESSAGE_HEADER_UPDATE_STATE) {
-            revert LCPCommitmentUnexpectedProxyMessageHeader();
-        }
-        return abi.decode(hm.message, (UpdateStateProxyMessage));
-    }
-
     struct MisbehaviourProxyMessage {
         PrevState[] prevStates;
         bytes context;
@@ -71,22 +55,6 @@ library LCPCommitment {
         bytes32 stateId;
     }
 
-    function parseMisbehaviourProxyMessage(bytes calldata messageBytes)
-        internal
-        pure
-        returns (MisbehaviourProxyMessage memory)
-    {
-        HeaderedProxyMessage memory hm = abi.decode(messageBytes, (HeaderedProxyMessage));
-        // MSB first
-        // 0-1:  version
-        // 2-3:  message type
-        // 4-31: reserved
-        if (hm.header != LCP_MESSAGE_HEADER_MISBEHAVIOUR) {
-            revert LCPCommitmentUnexpectedProxyMessageHeader();
-        }
-        return abi.decode(hm.message, (MisbehaviourProxyMessage));
-    }
-
     struct ValidationContext {
         bytes32 header;
         bytes context;

contracts/LCPOperator.sol

@@ -21,10 +21,12 @@ library LCPOperator {
     // chainTypeSalt(CHAIN_TYPE_EVM, hex"")
     bytes32 internal constant CHAIN_TYPE_EVM_SALT = keccak256(abi.encodePacked(CHAIN_TYPE_EVM, hex""));
 
+    // slither-disable-next-line dead-code
     function chainTypeSalt(ChainType chainType, bytes memory args) internal pure returns (bytes32) {
         return keccak256(abi.encodePacked(chainType, args));
     }
 
+    // slither-disable-next-line dead-code
     function domainSeparatorUniversal() internal pure returns (bytes32) {
         return keccak256(
             abi.encode(

contracts/LCPUtils.sol

@@ -17,7 +17,7 @@ library LCPUtils {
         pure
         returns (bytes memory bz, uint256 pos)
     {
-        pos = BytesUtils.find(src, offset, src.length, needle);
+        pos = BytesUtils.find(src, offset, src.length - offset, needle);
         if (pos == type(uint256).max) {
             revert LCPUtilsReadBytesUntilNotFound();
         }

scripts/gencert.sh

@@ -1,7 +1,6 @@
 #!/bin/env bash
 set -eu
 
-out_certs_dir=./test/.tmp/testcerts
 signing_rsa_bits=2048
 signing_exponent=65537
 
@@ -41,23 +40,24 @@ function gen_ecdsa_signing_cert() {
 }
 
 function usage() {
-    echo "Usage: $0 {gen_rsa_root_cert|gen_rsa_signing_cert|gen_ecdsa_root_cert|gen_ecdsa_signing_cert}"
+    echo "Usage: $0 <out_certs_dir> {gen_rsa_root_cert|gen_rsa_signing_cert|gen_ecdsa_root_cert|gen_ecdsa_signing_cert}"
     exit 1
 }
 
-if [ $# -eq 0 ]; then
+if [ $# -lt 2 ]; then
     usage
 fi
 
+out_certs_dir=$1
 mkdir -p ${out_certs_dir}
 
-case "$1" in
+case "$2" in
     "gen_rsa_root_cert")
         gen_rsa_root_cert
         ;;
     "gen_rsa_signing_cert")
-        signing_rsa_bits=$2
-        signing_exponent=$3
+        signing_rsa_bits=$3
+        signing_exponent=$4
         gen_rsa_signing_cert
         ;;
     "gen_ecdsa_root_cert")

slither.config.json

@@ -1,4 +1,4 @@
 {
-  "detectors_to_run": "arbitrary-send-erc20,array-by-reference,incorrect-shift,name-reused,rtlo,suicidal,uninitialized-storage,arbitrary-send-erc20-permit,controlled-array-length,controlled-delegatecall,delegatecall-loop,msg-value-loop,reentrancy-eth,unchecked-transfer,weak-prng,domain-separator-collision,erc20-interface,erc721-interface,locked-ether,mapping-deletion,shadowing-abstract,tautology,write-after-write,boolean-cst,reentrancy-no-eth,reused-constructor,tx-origin,unchecked-lowlevel,unchecked-send,variable-scope,void-cst,events-access,events-maths,incorrect-unary,boolean-equal,deprecated-standards,erc20-indexed,function-init-state,pragma,reentrancy-unlimited-gas,immutable-states,var-read-using-this",
+  "detectors_to_run": "arbitrary-send-erc20,array-by-reference,incorrect-shift,name-reused,rtlo,suicidal,uninitialized-storage,arbitrary-send-erc20-permit,controlled-array-length,controlled-delegatecall,delegatecall-loop,msg-value-loop,reentrancy-eth,unchecked-transfer,weak-prng,domain-separator-collision,erc20-interface,erc721-interface,locked-ether,mapping-deletion,shadowing-abstract,tautology,write-after-write,boolean-cst,reentrancy-no-eth,reused-constructor,tx-origin,unchecked-lowlevel,unchecked-send,variable-scope,void-cst,events-access,events-maths,incorrect-unary,boolean-equal,deprecated-standards,erc20-indexed,function-init-state,pragma,reentrancy-unlimited-gas,immutable-states,var-read-using-this,dead-code",
   "filter_paths": "(test/|node_modules/|contracts/proto/)"
 }