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..18f8fc1 100644 --- a/contracts/Asn1Decode.sol +++ b/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,14 +204,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,9 +221,10 @@ 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); } } +// slither-disable-end dead-code diff --git a/contracts/ILCPClientErrors.sol b/contracts/ILCPClientErrors.sol index 7ef8fee..be2b1dd 100644 --- a/contracts/ILCPClientErrors.sol +++ b/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(); diff --git a/contracts/LCPClientBase.sol b/contracts/LCPClientBase.sol index a0d707c..a104c3e 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 @@ -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); 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/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(); } 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/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/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); } diff --git a/test/LCPClientTest.t.sol b/test/LCPClientTest.t.sol index 5c2cb4a..9752963 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,19 @@ 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) + ); + require(success, "failed to update duplicated client"); + 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, 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)); } }