From b7c7d3367d29756d4485dc85a48a91dfaeaf4a4d Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Mon, 18 Nov 2024 23:33:20 +0900 Subject: [PATCH 01/10] fix out-of-boundary access in `LCPUtils::readBytesUntil()` Signed-off-by: Jun Kimura --- contracts/LCPUtils.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/LCPUtils.sol b/contracts/LCPUtils.sol index 6b85719..9692d59 100644 --- a/contracts/LCPUtils.sol +++ b/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(); } From 0f3591da0e8abd37b209d4c01d08ca8d0b4f0050 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 09:11:19 +0900 Subject: [PATCH 02/10] S2-1: add validations for `NotePtr.getPtr()` Signed-off-by: Jun Kimura --- contracts/Asn1Decode.sol | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) 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); } From a91d2b53ee01c8bbfbd7cde91cf2e001d8eaaa05 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 09:36:43 +0900 Subject: [PATCH 03/10] S2-2: clarify meaning of the quote status check Signed-off-by: Jun Kimura --- contracts/AVRValidator.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/AVRValidator.sol b/contracts/AVRValidator.sol index 73d2ec2..e58b49c 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 } } From 1cb681ce7b2ca72d4ce7ae9c1ef49f9f9db4ff2e Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 12:16:40 +0900 Subject: [PATCH 04/10] S2-6: fix to early return if the consensus state corresponding to `postHeight` Signed-off-by: Jun Kimura --- contracts/ILCPClientErrors.sol | 1 + contracts/LCPClientBase.sol | 19 +++++++++++++------ test/LCPClientTest.t.sol | 13 +++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) 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, From dec14e811b7d6affd0c5c7fe5b774449d9bf0340 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 14:59:40 +0900 Subject: [PATCH 05/10] S3-1: remove unnecessary offset calculation Signed-off-by: Jun Kimura --- contracts/AVRValidator.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/AVRValidator.sol b/contracts/AVRValidator.sol index e58b49c..b16857b 100644 --- a/contracts/AVRValidator.sol +++ b/contracts/AVRValidator.sol @@ -263,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" ); } From 87eed10ac27db010683141190dfafda70e5e21ba Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 15:36:36 +0900 Subject: [PATCH 06/10] S4: add gap storage Signed-off-by: Jun Kimura --- contracts/LCPClientBase.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/contracts/LCPClientBase.sol b/contracts/LCPClientBase.sol index e9e7dd8..614d702 100644 --- a/contracts/LCPClientBase.sol +++ b/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 From 5910e5551f3edc22bad8aefb5a7b158c7ceb8fd2 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 16:08:32 +0900 Subject: [PATCH 07/10] fix parallel test execution Signed-off-by: Jun Kimura --- scripts/gencert.sh | 12 ++++----- test/CertificateTest.t.sol | 55 +++++++++++++++++++++----------------- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/scripts/gencert.sh b/scripts/gencert.sh index f9781f7..bae21a6 100755 --- a/scripts/gencert.sh +++ b/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 {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") diff --git a/test/CertificateTest.t.sol b/test/CertificateTest.t.sol index f880615..ac88e95 100644 --- a/test/CertificateTest.t.sol +++ b/test/CertificateTest.t.sol @@ -71,7 +71,7 @@ contract CertificateTest is BasicTest { ); } - string internal constant testCertsDir = "./test/.tmp/testcerts"; + string internal constant testCertsBaseDir = "./test/.tmp/testcerts"; string internal constant genCertCmd = "./scripts/gencert.sh"; struct RSAParamsCase { @@ -80,6 +80,7 @@ contract CertificateTest is BasicTest { } function testValidSigningCerts() public { + string memory testCertsDir = string(abi.encodePacked(testCertsBaseDir, "/", "valid_signing_certs")); RSAParamsCase[4] memory cases = [ RSAParamsCase("2048", "65537"), RSAParamsCase("2048", "3"), @@ -87,10 +88,10 @@ contract CertificateTest is BasicTest { RSAParamsCase("4096", "3") ]; vm.warp(1795167418); - cleanupTestCerts(); - genRsaRootCert(); + cleanupTestCerts(testCertsDir); + genRsaRootCert(testCertsDir); for (uint256 i = 0; i < cases.length; i++) { - genRsaSigningCert(cases[i].bits, cases[i].exponent); + genRsaSigningCert(testCertsDir, cases[i].bits, cases[i].exponent); AVRValidator.RSAParams memory rootParams = AVRValidator.verifyRootCACert( vm.readFileBinary(string(abi.encodePacked(testCertsDir, "/root.crt.der"))) ); @@ -103,19 +104,21 @@ contract CertificateTest is BasicTest { } function testInvalidRootCerts() public { + string memory testCertsDir = string(abi.encodePacked(testCertsBaseDir, "/", "invalid_root_certs")); vm.warp(1795167418); - cleanupTestCerts(); - genEcdsaRootCert(); + cleanupTestCerts(testCertsDir); + genEcdsaRootCert(testCertsDir); vm.expectRevert(); AVRValidator.verifyRootCACert(vm.readFileBinary(string(abi.encodePacked(testCertsDir, "/root.crt.der")))); } function testInvalidSigningCerts() public { + string memory testCertsDir = string(abi.encodePacked(testCertsBaseDir, "/", "invalid_signing_certs")); vm.warp(1795167418); - cleanupTestCerts(); - genRsaRootCert(); - genEcdsaSigningCert(); + cleanupTestCerts(testCertsDir); + genRsaRootCert(testCertsDir); + genEcdsaSigningCert(testCertsDir); AVRValidator.RSAParams memory rootParams = AVRValidator.verifyRootCACert(vm.readFileBinary(string(abi.encodePacked(testCertsDir, "/root.crt.der")))); @@ -127,7 +130,7 @@ contract CertificateTest is BasicTest { ); } - function cleanupTestCerts() internal { + function cleanupTestCerts(string memory testCertsDir) internal { string[] memory cmd = new string[](3); cmd[0] = "rm"; cmd[1] = "-rf"; @@ -135,33 +138,37 @@ contract CertificateTest is BasicTest { runCmd(cmd); } - function genRsaRootCert() internal { - string[] memory cmd = new string[](2); + function genRsaRootCert(string memory testCertsDir) internal { + string[] memory cmd = new string[](3); cmd[0] = genCertCmd; - cmd[1] = "gen_rsa_root_cert"; + cmd[1] = testCertsDir; + cmd[2] = "gen_rsa_root_cert"; runCmd(cmd); } - function genRsaSigningCert(string memory bits, string memory exponent) internal { - string[] memory cmd = new string[](4); + function genRsaSigningCert(string memory testCertsDir, string memory bits, string memory exponent) internal { + string[] memory cmd = new string[](5); cmd[0] = genCertCmd; - cmd[1] = "gen_rsa_signing_cert"; - cmd[2] = bits; - cmd[3] = exponent; + cmd[1] = testCertsDir; + cmd[2] = "gen_rsa_signing_cert"; + cmd[3] = bits; + cmd[4] = exponent; runCmd(cmd); } - function genEcdsaRootCert() internal { - string[] memory cmd = new string[](2); + function genEcdsaRootCert(string memory testCertsDir) internal { + string[] memory cmd = new string[](3); cmd[0] = genCertCmd; - cmd[1] = "gen_ecdsa_root_cert"; + cmd[1] = testCertsDir; + cmd[2] = "gen_ecdsa_root_cert"; runCmd(cmd); } - function genEcdsaSigningCert() internal { - string[] memory cmd = new string[](2); + function genEcdsaSigningCert(string memory testCertsDir) internal { + string[] memory cmd = new string[](3); cmd[0] = genCertCmd; - cmd[1] = "gen_ecdsa_signing_cert"; + cmd[1] = testCertsDir; + cmd[2] = "gen_ecdsa_signing_cert"; runCmd(cmd); } From 86e5a24bf94d1a041e745a439566ffc491501c4d Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 17:02:42 +0900 Subject: [PATCH 08/10] tests: fix to check if staticcall is successful Signed-off-by: Jun Kimura --- test/LCPClientTest.t.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/test/LCPClientTest.t.sol b/test/LCPClientTest.t.sol index a0f6715..9752963 100644 --- a/test/LCPClientTest.t.sol +++ b/test/LCPClientTest.t.sol @@ -135,6 +135,7 @@ contract LCPClientTest is BasicTest { (bool success, bytes memory ret) = address(lc).staticcall( abi.encodeWithSelector(LCPClientBase.updateClient.selector, clientId, message) ); + require(success, "failed to update duplicated client"); heights = abi.decode(ret, (Height.Data[])); require(heights.length == 0, "heights length must be 0"); } From 30a68cb1750b66a82db65419f7ca35b43ea15d2a Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Wed, 20 Nov 2024 17:20:46 +0900 Subject: [PATCH 09/10] S8: fix to remove unused code Signed-off-by: Jun Kimura --- contracts/Asn1Decode.sol | 2 ++ contracts/ILCPClientErrors.sol | 1 - contracts/LCPClientBase.sol | 8 -------- contracts/LCPCommitment.sol | 32 -------------------------------- contracts/LCPOperator.sol | 2 ++ slither.config.json | 2 +- test/TestHelper.t.sol | 24 ++++++++++++++++++++---- 7 files changed, 25 insertions(+), 46 deletions(-) diff --git a/contracts/Asn1Decode.sol b/contracts/Asn1Decode.sol index 97ca1ed..18f8fc1 100644 --- a/contracts/Asn1Decode.sol +++ b/contracts/Asn1Decode.sol @@ -55,6 +55,7 @@ library NodePtr { } } +// slither-disable-start dead-code library Asn1Decode { using NodePtr for uint256; using BytesUtils for bytes; @@ -226,3 +227,4 @@ library Asn1Decode { return NodePtr.getPtr(ix, ixFirstContentByte, ixLastContentByte); } } +// slither-disable-end dead-code diff --git a/contracts/ILCPClientErrors.sol b/contracts/ILCPClientErrors.sol index ab98229..aa41eaf 100644 --- a/contracts/ILCPClientErrors.sol +++ b/contracts/ILCPClientErrors.sol @@ -8,7 +8,6 @@ interface ILCPClientErrors { error LCPClientClientStateInvalidKeyExpiration(); error LCPClientClientStateInvalidMrenclaveLength(); error LCPClientClientStateUnexpectedMrenclave(); - error LCPClientClientStateEmptyOperators(); error LCPClientClientStateInvalidOperatorAddress(); error LCPClientClientStateInvalidOperatorAddressLength(); error LCPClientClientStateInvalidOperatorsNonce(); diff --git a/contracts/LCPClientBase.sol b/contracts/LCPClientBase.sol index 614d702..e6e215c 100644 --- a/contracts/LCPClientBase.sol +++ b/contracts/LCPClientBase.sol @@ -650,14 +650,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); diff --git a/contracts/LCPCommitment.sol b/contracts/LCPCommitment.sol index ad18f95..8a6b0ae 100644 --- a/contracts/LCPCommitment.sol +++ b/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; diff --git a/contracts/LCPOperator.sol b/contracts/LCPOperator.sol index fd48f04..adca35a 100644 --- a/contracts/LCPOperator.sol +++ b/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( diff --git a/slither.config.json b/slither.config.json index bbcdead..80ba075 100644 --- a/slither.config.json +++ b/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/)" } \ No newline at end of file diff --git a/test/TestHelper.t.sol b/test/TestHelper.t.sol index 068f424..2faa9e2 100644 --- a/test/TestHelper.t.sol +++ b/test/TestHelper.t.sol @@ -108,12 +108,20 @@ library LCPCommitmentTestHelper { LCPCommitment.trustingPeriodContextEval(context, currentTimestampNanos); } - function parseUpdateStateProxyMessage(bytes calldata commitmentBytes) + function parseUpdateStateProxyMessage(bytes calldata messageBytes) public pure - returns (LCPCommitment.UpdateStateProxyMessage memory commitment) + returns (LCPCommitment.UpdateStateProxyMessage memory) { - return LCPCommitment.parseUpdateStateProxyMessage(commitmentBytes); + LCPCommitment.HeaderedProxyMessage memory hm = abi.decode(messageBytes, (LCPCommitment.HeaderedProxyMessage)); + // MSB first + // 0-1: version + // 2-3: message type + // 4-31: reserved + if (hm.header != LCPCommitment.LCP_MESSAGE_HEADER_UPDATE_STATE) { + revert LCPCommitment.LCPCommitmentUnexpectedProxyMessageHeader(); + } + return abi.decode(hm.message, (LCPCommitment.UpdateStateProxyMessage)); } function parseVerifyMembershipCommitmentProofs(bytes calldata proofBytes) @@ -129,7 +137,15 @@ library LCPCommitmentTestHelper { pure returns (LCPCommitment.MisbehaviourProxyMessage memory) { - return LCPCommitment.parseMisbehaviourProxyMessage(messageBytes); + LCPCommitment.HeaderedProxyMessage memory hm = abi.decode(messageBytes, (LCPCommitment.HeaderedProxyMessage)); + // MSB first + // 0-1: version + // 2-3: message type + // 4-31: reserved + if (hm.header != LCPCommitment.LCP_MESSAGE_HEADER_MISBEHAVIOUR) { + revert LCPCommitment.LCPCommitmentUnexpectedProxyMessageHeader(); + } + return abi.decode(hm.message, (LCPCommitment.MisbehaviourProxyMessage)); } } From bf2266a5a1ca0ef5824b89b6ee8ef00919dd6f22 Mon Sep 17 00:00:00 2001 From: Jun Kimura Date: Tue, 26 Nov 2024 21:06:30 +0900 Subject: [PATCH 10/10] S2-3: improve validations for `clientState.allowed_quote_statuses` and `clientState.allowed_advisory_ids` Signed-off-by: Jun Kimura --- contracts/ILCPClientErrors.sol | 2 ++ contracts/LCPClientBase.sol | 14 ++++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/contracts/ILCPClientErrors.sol b/contracts/ILCPClientErrors.sol index aa41eaf..be2b1dd 100644 --- a/contracts/ILCPClientErrors.sol +++ b/contracts/ILCPClientErrors.sol @@ -12,6 +12,8 @@ interface ILCPClientErrors { error LCPClientClientStateInvalidOperatorAddressLength(); error LCPClientClientStateInvalidOperatorsNonce(); error LCPClientClientStateUnexpectedOperatorsNonce(uint64 expectedNonce); + error LCPClientClientStateInvalidAllowedQuoteStatus(); + error LCPClientClientStateInvalidAllowedAdvisoryId(); error LCPClientOperatorsInvalidOrder(address prevOperator, address nextOperator); error LCPClientClientStateInvalidOperatorsThreshold(); diff --git a/contracts/LCPClientBase.sol b/contracts/LCPClientBase.sol index e6e215c..a104c3e 100644 --- a/contracts/LCPClientBase.sol +++ b/contracts/LCPClientBase.sol @@ -169,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;