From d081d4c9f1b21d653ffb3105aaf1702c0c23532b Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Tue, 25 Feb 2025 22:48:10 +0200 Subject: [PATCH 1/5] feat: cleanup `BytecodeRepository` --- contracts/global/BytecodeRepository.sol | 836 +++++++++--------- contracts/instance/PriceFeedStore.sol | 2 +- contracts/interfaces/IBytecodeRepository.sol | 298 +++---- contracts/interfaces/Types.sol | 74 +- contracts/market/MarketConfigurator.sol | 2 +- .../MarketConfigurator.unit.t.sol | 13 +- .../test/global/BytecodeRepository.unit.t.sol | 56 +- .../test/global/PriceFeedStore.unit.t.sol | 24 +- contracts/test/helpers/BCRHelpers.sol | 11 +- 9 files changed, 623 insertions(+), 693 deletions(-) diff --git a/contracts/global/BytecodeRepository.sol b/contracts/global/BytecodeRepository.sol index 55db9b9..962a838 100644 --- a/contracts/global/BytecodeRepository.sol +++ b/contracts/global/BytecodeRepository.sol @@ -4,569 +4,555 @@ pragma solidity ^0.8.23; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; -import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; -import {IBytecodeRepository} from "../interfaces/IBytecodeRepository.sol"; -import {AP_BYTECODE_REPOSITORY} from "../libraries/ContractLiterals.sol"; -import {IVersion} from "@gearbox-protocol/core-v3/contracts/interfaces/base/IVersion.sol"; -import {SanityCheckTrait} from "@gearbox-protocol/core-v3/contracts/traits/SanityCheckTrait.sol"; -import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; + import {LibString} from "@solady/utils/LibString.sol"; -import "@gearbox-protocol/core-v3/contracts/interfaces/IExceptions.sol"; +import {SSTORE2} from "@solady/utils/SSTORE2.sol"; + +import {IVersion} from "@gearbox-protocol/core-v3/contracts/interfaces/base/IVersion.sol"; +import {SanityCheckTrait} from "@gearbox-protocol/core-v3/contracts/traits/SanityCheckTrait.sol"; -import {Bytecode, BytecodePointer, AuditorSignature} from "../interfaces/Types.sol"; import {EIP712Mainnet} from "../helpers/EIP712Mainnet.sol"; +import {IBytecodeRepository} from "../interfaces/IBytecodeRepository.sol"; +import {AuditReport, Bytecode, BytecodePointer} from "../interfaces/Types.sol"; +import {AP_BYTECODE_REPOSITORY} from "../libraries/ContractLiterals.sol"; import {Domain} from "../libraries/Domain.sol"; import {ImmutableOwnableTrait} from "../traits/ImmutableOwnableTrait.sol"; -import {SSTORE2} from "@solady/utils/SSTORE2.sol"; -/** - * @title BytecodeRepository - * - * @notice - * The `BytecodeRepository` is a singleton contract responsible for deploying all contracts in the system for risk curators. - * - * Each contract is identified by two parameters: - * - ContractType: bytes32. Can be further split into two parts: - * - Domain: Represents the fundamental category or name of the smart contract. For example, - * contracts like `Pools` or `CreditManagers` use the contract name as the domain. - * - Postfix: For contracts that offer different implementations under the same interface - * (such as interest rate models, adapters, or price feeds), the domain remains fixed. - * Variations are distinguished using a postfix. This is established by convention. - * - Version: uint256: Specifies the version of the contract in semver. (example: 3_10) - * - * ContractType Convention: - * - The contract type follows a capitalized snake_case naming convention - * without any version information (example: "POOL", "CREDIT_MANAGER") - * - List of domains: - * RK_ - rate keepers. Supports IRateKeeperBase interface. - * IRM_ - interest rate models - * - * This structure ensures consistency and clarity when deploying and managing contracts within the system. - */ +/// @title Bytecode repository contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecodeRepository, EIP712Mainnet { using EnumerableSet for EnumerableSet.AddressSet; using EnumerableSet for EnumerableSet.Bytes32Set; - using ECDSA for bytes32; using LibString for bytes32; using LibString for string; using LibString for uint256; - using Domain for string; - using SSTORE2 for bytes; - // - // CONSTANTS - // + using Domain for bytes32; + + /// @dev Internal struct with version info for a given contract type + struct VersionInfo { + uint256 latest; + mapping(uint256 => uint256) latestByMajor; + mapping(uint256 => uint256) latestByMinor; + } - /// @notice Meta info about contract type & version + /// @notice Contract version uint256 public constant override version = 3_10; + + /// @notice Contract type bytes32 public constant override contractType = AP_BYTECODE_REPOSITORY; - bytes32 public constant BYTECODE_TYPEHASH = + /// @notice Bytecode typehash + bytes32 public constant override BYTECODE_TYPEHASH = keccak256("Bytecode(bytes32 contractType,uint256 version,bytes initCode,address author,string source)"); - bytes32 public constant AUDITOR_SIGNATURE_TYPEHASH = - keccak256("SignBytecodeHash(bytes32 bytecodeHash,string reportUrl)"); - - // - // STORAGE - // + /// @notice Audit report typehash + bytes32 public constant override AUDIT_REPORT_TYPEHASH = + keccak256("AuditReport(bytes32 bytecodeHash,address auditor,string reportUrl)"); - // bytecodeHash => Bytecode - mapping(bytes32 => BytecodePointer) internal _bytecodeByHash; + /// @dev Mapping from `deployedContract` deployed from the repository to its bytecode hash + mapping(address deployedContract => bytes32) _deployedContractBytecodeHashes; - // bytecodeHash => array of AuditorSignature - mapping(bytes32 => AuditorSignature[]) internal _auditorSignaturesByHash; + /// @dev Mapping from `bytecodeHash` to pointer to bytecode with given hash + mapping(bytes32 bytecodeHash => BytecodePointer) internal _bytecodeByHash; - EnumerableSet.Bytes32Set internal _bytecodeHashes; + /// @dev Mapping from `bytecodeHash` to its audit reports + mapping(bytes32 bytecodeHash => AuditReport[]) internal _auditReports; - // contractType => version => bytecodeHash - mapping(bytes32 => mapping(uint256 => bytes32)) public approvedBytecodeHash; + /// @dev Mapping from `cType` to `ver` to allowed bytecode hash + mapping(bytes32 cType => mapping(uint256 ver => bytes32 bytecodeHash)) internal _allowedBytecodeHashes; - // address => bytecodeHash - mapping(address => bytes32) public deployedContracts; + /// @dev Mapping from `cType` to its owner + mapping(bytes32 cType => address owner) internal _contractTypeOwners; - // Forbidden initCodes - mapping(bytes32 => bool) public forbiddenInitCode; + /// @dev Mapping from `bytecodeHash` to whether it is allowed as system contract + mapping(bytes32 bytecodeHash => bool) internal _isAllowedSystemContract; - // Allowed system contracts - mapping(bytes32 => bool) public allowedSystemContracts; + /// @dev Set of public domains + EnumerableSet.Bytes32Set internal _publicDomainsSet; - // Distinguish system vs. public domains - EnumerableSet.Bytes32Set private _publicDomains; + /// @dev Set of approved auditors + EnumerableSet.AddressSet internal _auditorsSet; - // if contractType is public - mapping(bytes32 => address) public contractTypeOwner; + /// @dev Mapping from `auditor` to their name + mapping(address auditor => string) internal _auditorNames; - // Auditors - EnumerableSet.AddressSet private _auditors; + /// @dev Mapping from `initCodeHash` to whether it is forbidden + mapping(bytes32 initCodeHash => bool) internal _isInitCodeForbidden; - // Auditor => name - mapping(address => string) public auditorName; + /// @dev Mapping from `token` to its specific postfix + mapping(address token => bytes32) internal _tokenSpecificPostfixes; - // Postfixes are used to deploy unique contract versions inherited from - // the base contract but differ when used with specific tokens. - // For example, the USDT pool, which supports fee computation without errors - mapping(address => bytes32) public tokenSpecificPostfixes; + /// @dev Mapping from `cType` to its version info + mapping(bytes32 cType => VersionInfo) internal _versionInfo; - // Version control - mapping(bytes32 => uint256) public latestVersion; - mapping(bytes32 => mapping(uint256 => uint256)) public latestMinorVersion; - mapping(bytes32 => mapping(uint256 => uint256)) public latestPatchVersion; - - constructor(address _owner) + /// @notice Constructor + /// @param owner_ Owner of the bytecode repository + constructor(address owner_) EIP712Mainnet(contractType.fromSmallString(), version.toString()) - ImmutableOwnableTrait(_owner) + ImmutableOwnableTrait(owner_) {} - /// @notice Computes a unique hash for _bytecode metadata - /// @param _bytecode Bytecode metadata including contract type, version, _bytecode, author and source - /// @return bytes32 Hash of the metadata - function computeBytecodeHash(Bytecode memory _bytecode) public pure returns (bytes32) { + // --------------- // + // EIP-712 GETTERS // + // --------------- // + + /// @notice Returns the domain separator + function domainSeparatorV4() external view override returns (bytes32) { + return _domainSeparatorV4(); + } + + /// @notice Computes bytecode's struct hash + function computeBytecodeHash(Bytecode calldata bytecode) public pure override returns (bytes32) { return keccak256( abi.encode( BYTECODE_TYPEHASH, - _bytecode.contractType, - _bytecode.version, - keccak256(_bytecode.initCode), - _bytecode.author, - keccak256(bytes(_bytecode.source)) + bytecode.contractType, + bytecode.version, + keccak256(bytecode.initCode), + bytecode.author, + keccak256(bytes(bytecode.source)) ) ); } - /// @notice Uploads new _bytecode to the repository - /// @param _bytecode Bytecode metadata to upload - /// @dev Only the author can upload on mainnet - function uploadBytecode(Bytecode memory _bytecode) external nonZeroAddress(_bytecode.author) { - if (block.chainid == 1 && msg.sender != _bytecode.author) { - revert OnlyAuthorCanSyncException(); - } - // Check if _bytecode is already uploaded - bytes32 bytecodeHash = computeBytecodeHash(_bytecode); - - if (isBytecodeUploaded(bytecodeHash)) { - revert BytecodeAlreadyExistsException(); - } - - // Verify author's signature of the _bytecode metadata - address recoveredAuthor = ECDSA.recover(_hashTypedDataV4(bytecodeHash), _bytecode.authorSignature); - if (recoveredAuthor != _bytecode.author) { - revert InvalidAuthorSignatureException(); - } - - // Revert if the initCode is forbidden - revertIfInitCodeForbidden(_bytecode.initCode); + /// @notice Computes struct hash for auditor signature + function computeAuditReportHash(bytes32 bytecodeHash, address auditor, string calldata reportUrl) + public + pure + override + returns (bytes32) + { + return keccak256(abi.encode(AUDIT_REPORT_TYPEHASH, bytecodeHash, auditor, keccak256(bytes(reportUrl)))); + } - // Check if the contract name and version already exists - if (approvedBytecodeHash[_bytecode.contractType][_bytecode.version] != 0) { - revert ContractTypeVersionAlreadyExistsException(); - } + // ------------------- // + // DEPLOYING CONTRACTS // + // ------------------- // - address initCodePointer = _bytecode.initCode.write(); + /// @notice Whether `deployedContract` was deployed from the repository + function isDeployedFromRepository(address deployedContract) external view override returns (bool) { + return _deployedContractBytecodeHashes[deployedContract] != bytes32(0); + } - _bytecodeByHash[bytecodeHash] = BytecodePointer({ - contractType: _bytecode.contractType, - version: _bytecode.version, - initCodePointer: initCodePointer, - author: _bytecode.author, - source: _bytecode.source, - authorSignature: _bytecode.authorSignature - }); + /// @notice Returns bytecode hash for `deployedContract` deployed from the repository + function getDeployedContractBytecodeHash(address deployedContract) external view override returns (bytes32) { + return _deployedContractBytecodeHashes[deployedContract]; + } - _bytecodeHashes.add(bytecodeHash); + /// @notice Computes the address at which a contract of a given type and version + /// with given constructor parameters and salt would be deployed + /// @dev Deployer's address is mixed with salt to prevent front-running using collisions + function computeAddress(bytes32 cType, uint256 ver, bytes memory constructorParams, bytes32 salt, address deployer) + external + view + override + returns (address) + { + bytes32 bytecodeHash = _allowedBytecodeHashes[cType][ver]; + BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; + bytes memory initCode = SSTORE2.read(bytecode.initCodePointer); - emit UploadBytecode( - bytecodeHash, - _bytecode.contractType.fromSmallString(), - _bytecode.version, - _bytecode.author, - _bytecode.source - ); + bytes32 uniqueSalt = keccak256(abi.encode(salt, deployer)); + bytes memory bytecodeWithParams = abi.encodePacked(initCode, constructorParams); + return Create2.computeAddress(uniqueSalt, keccak256(bytecodeWithParams)); } - /// @notice Deploys a contract using stored _bytecode - /// @param _contractType Type identifier of the contract - /// @param _version Version of the contract to deploy - /// @param constructorParams Constructor parameters for deployment - /// @param salt Unique salt for CREATE2 deployment - /// @return newContract Address of the deployed contract - function deploy(bytes32 _contractType, uint256 _version, bytes memory constructorParams, bytes32 salt) + /// @notice Deploys a contract of a given type and version with given constructor parameters and salt. + /// Tries to transfer ownership over the deployed contract to the caller. + /// Bytecode must be allowed, which means it is either a system contract or a contract from public + /// domain with at least one signed report from approved auditor and not forbidden init code. + /// @dev Deployer's address is mixed with salt to prevent front-running using collisions + /// @dev Reverts if contract was previously deployed at the same address + /// @dev Reverts if deployed contract's type or version does not match passed parameters + function deploy(bytes32 cType, uint256 ver, bytes memory constructorParams, bytes32 salt) external + override returns (address newContract) { - // Retrieve bytecodeHash - bytes32 bytecodeHash = approvedBytecodeHash[_contractType][_version]; - if (bytecodeHash == 0) { - revert BytecodeIsNotApprovedException(_contractType, _version); - } - - if (!isAuditBytecode(bytecodeHash)) { - revert BytecodeIsNotAuditedException(); - } - - BytecodePointer storage _bytecode = _bytecodeByHash[bytecodeHash]; + bytes32 bytecodeHash = _allowedBytecodeHashes[cType][ver]; + if (bytecodeHash == 0) revert BytecodeIsNotAllowedException(cType, ver); - bytes memory initCode = SSTORE2.read(_bytecode.initCodePointer); + BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; + bytes memory initCode = SSTORE2.read(bytecode.initCodePointer); + _revertIfInitCodeIsForbidden(initCode); - // Revert if the initCode is forbidden - revertIfInitCodeForbidden(initCode); - - // Combine code + constructor params + bytes32 uniqueSalt = keccak256(abi.encode(salt, msg.sender)); bytes memory bytecodeWithParams = abi.encodePacked(initCode, constructorParams); + newContract = Create2.computeAddress(uniqueSalt, keccak256(bytecodeWithParams)); - bytes32 saltUnique = keccak256(abi.encode(salt, msg.sender)); - - // Compute CREATE2 address - newContract = Create2.computeAddress(saltUnique, keccak256(bytecodeWithParams)); - - // Check if the contract already deployed - if (newContract.code.length != 0) { - revert BytecodeAlreadyExistsAtAddressException(newContract); + if (newContract.code.length != 0) revert ContractIsAlreadyDeployedException(newContract); + Create2.deploy(0, uniqueSalt, bytecodeWithParams); + if (IVersion(newContract).contractType() != cType || IVersion(newContract).version() != ver) { + revert InvalidBytecodeException(bytecodeHash); } - // Deploy - Create2.deploy(0, saltUnique, bytecodeWithParams); + _deployedContractBytecodeHashes[newContract] = bytecodeHash; + emit DeployContract(bytecodeHash, cType, ver, newContract); - // Verify IVersion - if (IVersion(newContract).contractType() != _contractType || IVersion(newContract).version() != _version) { - revert IncorrectBytecodeException(bytecodeHash); - } + try Ownable(newContract).transferOwnership(msg.sender) {} catch {} + } - // add to deployedContracts - deployedContracts[newContract] = bytecodeHash; + // ------------------ // + // UPLOADING BYTECODE // + // ------------------ // - emit DeployContract(newContract, bytecodeHash, _contractType.fromSmallString(), _version); + /// @notice Returns bytecode with `bytecodeHash` + /// @dev Reverts if bytecode is not uploaded + function getBytecode(bytes32 bytecodeHash) external view override returns (Bytecode memory) { + BytecodePointer memory bytecode = _bytecodeByHash[bytecodeHash]; + if (bytecode.initCodePointer == address(0)) revert BytecodeIsNotUploadedException(bytecodeHash); + return Bytecode({ + contractType: bytecode.contractType, + version: bytecode.version, + initCode: SSTORE2.read(bytecode.initCodePointer), + author: bytecode.author, + source: bytecode.source, + authorSignature: bytecode.authorSignature + }); + } - // Auto-transfer ownership if IOwnable - try Ownable(newContract).transferOwnership(msg.sender) {} catch {} + /// @notice Whether bytecode with `bytecodeHash` is uploaded + function isBytecodeUploaded(bytes32 bytecodeHash) public view override returns (bool) { + return _bytecodeByHash[bytecodeHash].initCodePointer != address(0); } - /// @notice Computes the address where a contract would be deployed - /// @param _contractType Type identifier of the contract - /// @param _version Version of the contract - /// @param constructorParams Constructor parameters - /// @param salt Unique salt for CREATE2 deployment - /// @return Address where the contract would be deployed - function computeAddress( - bytes32 _contractType, - uint256 _version, - bytes memory constructorParams, - bytes32 salt, - address deployer - ) external view returns (address) { - // Retrieve bytecodeHash - bytes32 bytecodeHash = approvedBytecodeHash[_contractType][_version]; - if (bytecodeHash == 0) { - revert BytecodeIsNotApprovedException(_contractType, _version); + /// @notice Uploads new contract bytecode to the repository. + /// Simply uploading the bytecode is not enough to deploy a contract with it, see `deploy` for details. + /// @dev Reverts if bytecode for given contract type and version is already allowed + /// @dev Reverts if author is zero address or if their signature is invalid + /// @dev On mainnet, only author of the bytecode can upload it + function uploadBytecode(Bytecode calldata bytecode) external override nonZeroAddress(bytecode.author) { + bytes32 bytecodeHash = computeBytecodeHash(bytecode); + if (isBytecodeUploaded(bytecodeHash)) return; + + if (_allowedBytecodeHashes[bytecode.contractType][bytecode.version] != 0) { + revert BytecodeIsAlreadyAllowedException(bytecode.contractType, bytecode.version); } - BytecodePointer storage _bytecode = _bytecodeByHash[bytecodeHash]; - bytes memory initCode = SSTORE2.read(_bytecode.initCodePointer); + if (block.chainid == 1 && msg.sender != bytecode.author) revert CallerIsNotBytecodeAuthorException(msg.sender); + address author = ECDSA.recover(_hashTypedDataV4(bytecodeHash), bytecode.authorSignature); + if (author != bytecode.author) revert InvalidAuthorSignatureException(author); - // Combine code + constructor params - bytes memory bytecodeWithParams = abi.encodePacked(initCode, constructorParams); + address initCodePointer = SSTORE2.write(bytecode.initCode); + _bytecodeByHash[bytecodeHash] = BytecodePointer({ + contractType: bytecode.contractType, + version: bytecode.version, + initCodePointer: initCodePointer, + author: bytecode.author, + source: bytecode.source, + authorSignature: bytecode.authorSignature + }); + emit UploadBytecode(bytecodeHash, bytecode.contractType, bytecode.version, bytecode.author, bytecode.source); + } - bytes32 saltUnique = keccak256(abi.encode(salt, deployer)); + // ----------------- // + // AUDITING BYTECODE // + // ----------------- // - // Return CREATE2 address - return Create2.computeAddress(saltUnique, keccak256(bytecodeWithParams)); + /// @notice Whether bytecode with `bytecodeHash` is signed at least by one approved auditor + function isBytecodeAudited(bytes32 bytecodeHash) public view override returns (bool) { + uint256 len = _auditReports[bytecodeHash].length; + for (uint256 i; i < len; ++i) { + AuditReport memory report = _auditReports[bytecodeHash][i]; + if (isAuditor(report.auditor)) return true; + } + return false; } - /// @notice Allows auditors to sign _bytecode metadata - /// @param bytecodeHash Hash of the _bytecode metadata to sign - /// @param reportUrl URL of the audit report - /// @param signature Cryptographic signature of the auditor - function signBytecodeHash(bytes32 bytecodeHash, string calldata reportUrl, bytes memory signature) external { - // Must point to existing metadata - if (!isBytecodeUploaded(bytecodeHash)) { - revert BytecodeIsNotUploadedException(bytecodeHash); - } + /// @notice Returns all audit reports for `bytecodeHash` + function getAuditReports(bytes32 bytecodeHash) external view override returns (AuditReport[] memory) { + return _auditReports[bytecodeHash]; + } - // Re-create typed data - bytes32 structHash = - keccak256(abi.encode(AUDITOR_SIGNATURE_TYPEHASH, bytecodeHash, keccak256(bytes(reportUrl)))); - // Hash with our pinned domain - address signer = ECDSA.recover(_hashTypedDataV4(structHash), signature); + /// @notice Returns audit report at `index` for `bytecodeHash` + function getAuditReport(bytes32 bytecodeHash, uint256 index) external view override returns (AuditReport memory) { + return _auditReports[bytecodeHash][index]; + } - // Must match msg.sender and be an approved auditor - if (!_auditors.contains(signer)) { - revert SignerIsNotAuditorException(signer); - } + /// @notice Returns number of audit reports for `bytecodeHash` + function getNumAuditReports(bytes32 bytecodeHash) external view override returns (uint256) { + return _auditReports[bytecodeHash].length; + } - // do not allow duplicates - uint256 len = _auditorSignaturesByHash[bytecodeHash].length; - for (uint256 i = 0; i < len; ++i) { - if (keccak256(_auditorSignaturesByHash[bytecodeHash][i].signature) == keccak256(signature)) { - revert AuditorAlreadySignedException(); + /// @notice Submits signed audit report for bytecode with `bytecodeHash` + /// @dev Reverts if bytecode is not uploaded + /// @dev Reverts if auditor is not approved, already signed bytecode, or their signature is invalid + function submitAuditReport(bytes32 bytecodeHash, AuditReport calldata auditReport) external override { + if (!isBytecodeUploaded(bytecodeHash)) revert BytecodeIsNotUploadedException(bytecodeHash); + if (!_auditorsSet.contains(auditReport.auditor)) revert AuditorIsNotApprovedException(auditReport.auditor); + + bytes32 reportHash = computeAuditReportHash(bytecodeHash, auditReport.auditor, auditReport.reportUrl); + address auditor = ECDSA.recover(_hashTypedDataV4(reportHash), auditReport.signature); + if (auditor != auditReport.auditor) revert InvalidAuditorSignatureException(auditor); + + AuditReport[] storage reports = _auditReports[bytecodeHash]; + uint256 len = reports.length; + for (uint256 i; i < len; ++i) { + if (keccak256(reports[i].signature) == keccak256(auditReport.signature)) { + revert BytecodeIsAlreadySignedByAuditorException(bytecodeHash, auditor); } } - _auditorSignaturesByHash[bytecodeHash].push( - AuditorSignature({reportUrl: reportUrl, auditor: signer, signature: signature}) - ); - - emit AuditBytecode(bytecodeHash, signer, reportUrl, signature); - - BytecodePointer storage _bytecode = _bytecodeByHash[bytecodeHash]; + reports.push(auditReport); + emit AuditBytecode(bytecodeHash, auditor, auditReport.reportUrl); - bytes32 _contractType = _bytecode.contractType; - address author = _bytecode.author; - - bool isSystemContract = allowedSystemContracts[bytecodeHash]; + _doStuff(bytecodeHash); + } - address currentOwner = contractTypeOwner[_contractType]; - bool isContractTypeInPublicDomain = isInPublicDomain(_contractType); + // ----------------- // + // ALLOWING BYTECODE // + // ----------------- // + + // TODO: rework this section + // some issues: + // - nothing prevents owner from calling `allowSystemContract` twice for the same contract type and version, + // overwriting the contract type owner + // - the order in which `addPublicDomain` and `submitAuditReport` are called is important as submitting the + // report before adding the public domain leaves us with unallowed contract unless we directly allow it + // as system contract or find another auditor signature (with fake report URL or idk) + // - the behavior is a bit unpredictable if owner allows system contract which has a type from public domain + + /// @notice Returns the allowed bytecode hash for `cType` and `ver` + function getAllowedBytecodeHash(bytes32 cType, uint256 ver) external view override returns (bytes32) { + return _allowedBytecodeHashes[cType][ver]; + } - if (currentOwner == address(0) || isSystemContract) { - contractTypeOwner[_contractType] = author; - } else if (isContractTypeInPublicDomain && (currentOwner != author)) { - revert NotDomainOwnerException(); - } + /// @notice Returns the owner of `cType` + function getContractTypeOwner(bytes32 cType) external view override returns (address) { + return _contractTypeOwners[cType]; + } - if (isSystemContract || isContractTypeInPublicDomain) { - _approveContract(_contractType, _bytecode.version, bytecodeHash); - } + /// @notice Returns whether contract with `bytecodeHash` is an allowed system contract + function isAllowedSystemContract(bytes32 bytecodeHash) external view override returns (bool) { + return _isAllowedSystemContract[bytecodeHash]; } /// @notice Allows owner to mark contracts as system contracts - /// @param bytecodeHash Hash of the _bytecode metadata to allow - function allowSystemContract(bytes32 bytecodeHash) external onlyOwner { - allowedSystemContracts[bytecodeHash] = true; - - if (isBytecodeUploaded(bytecodeHash) && isAuditBytecode(bytecodeHash)) { - BytecodePointer storage _bytecode = _bytecodeByHash[bytecodeHash]; - contractTypeOwner[_bytecode.contractType] = _bytecode.author; - _approveContract(_bytecode.contractType, _bytecode.version, bytecodeHash); - } + /// @param bytecodeHash Hash of the bytecode metadata to allow + function allowSystemContract(bytes32 bytecodeHash) external override onlyOwner { + if (!isBytecodeUploaded(bytecodeHash) || !isBytecodeAudited(bytecodeHash)) return; + if (_isAllowedSystemContract[bytecodeHash]) return; + + _isAllowedSystemContract[bytecodeHash] = true; + BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; + bytes32 cType = bytecode.contractType; + address author = bytecode.author; + + // QUESTION: should we check if `cType` already has an owner? + // because it's in public domain or was previously allowed as system contract + _contractTypeOwners[cType] = author; + emit SetContractTypeOwner(cType, author); + + _allowBytecode(bytecodeHash, cType, bytecode.version); } - /// @notice Internal function to approve contract _bytecode - /// @param bytecodeHash Hash of the _bytecode metadata to approve - function _approveContract(bytes32 _contractType, uint256 _version, bytes32 bytecodeHash) internal { - if (approvedBytecodeHash[_contractType][_version] == 0) { - approvedBytecodeHash[_contractType][_version] = bytecodeHash; - - uint256 majorVersion = (_version / 100) * 100; - uint256 minorVersion = ((_version / 10) % 10) * 10 + majorVersion; - - if (latestVersion[_contractType] < _version) { - latestVersion[_contractType] = _version; - } - if (latestMinorVersion[_contractType][majorVersion] < _version) { - latestMinorVersion[_contractType][majorVersion] = _version; - } - if (latestPatchVersion[_contractType][minorVersion] < _version) { - latestPatchVersion[_contractType][minorVersion] = _version; - } - - emit ApproveContract(bytecodeHash, _contractType, _version); + function revokeApproval(bytes32 cType, uint256 ver, bytes32 bytecodeHash) external override onlyOwner { + if (_allowedBytecodeHashes[cType][ver] == bytecodeHash) { + _allowedBytecodeHashes[cType][ver] = bytes32(0); + emit ForbidBytecode(bytecodeHash, cType, ver); } } - // - // Auditor management - // - /// @notice Adds a new auditor - /// @param auditor Address of the auditor - /// @param name Name of the auditor - function addAuditor(address auditor, string memory name) external onlyOwner nonZeroAddress(auditor) { - bool added = _auditors.add(auditor); - if (added) { - auditorName[auditor] = name; - emit AddAuditor(auditor, name); - } + /// @notice Removes contract type owner + /// @param cType Contract type to remove owner from + /// @dev Used to remove malicious auditors and cybersquatters + function removeContractTypeOwner(bytes32 cType) external override onlyOwner { + if (_contractTypeOwners[cType] == address(0)) return; + _contractTypeOwners[cType] = address(0); + emit RemoveContractTypeOwner(cType); } - /// @notice Removes an auditor - /// @param auditor Address of the auditor to remove - function removeAuditor(address auditor) external onlyOwner { - bool removed = _auditors.remove(auditor); - if (removed) { - emit RemoveAuditor(auditor); + function _doStuff(bytes32 bytecodeHash) internal { + BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; + bytes32 cType = bytecode.contractType; + + bool isSystemContract = _isAllowedSystemContract[bytecodeHash]; + bool isContractTypeInPublicDomain = isInPublicDomain(cType); + + address author = bytecode.author; + address currentOwner = _contractTypeOwners[cType]; + if (isSystemContract && currentOwner == address(0)) { + _contractTypeOwners[cType] = author; + emit SetContractTypeOwner(cType, author); + } else if (isContractTypeInPublicDomain && currentOwner != author) { + revert AuthorIsNotContractTypeOwnerException(cType, author); } - } - /// @notice Checks if an address is an approved auditor - /// @param auditor Address to check - /// @return bool True if address is an approved auditor - function isAuditor(address auditor) public view returns (bool) { - return _auditors.contains(auditor); + // FIXME: for contracts from public domain, submitting a signature before adding the public domain leaves us + // with no other way to later allow it except marking it as system contract or finding another auditor signature + if (isSystemContract || isContractTypeInPublicDomain) _allowBytecode(bytecodeHash, cType, bytecode.version); } - /// @notice Returns list of all approved auditors - /// @return Array of auditor addresses - function getAuditors() external view returns (address[] memory) { - return _auditors.values(); - } + function _allowBytecode(bytes32 bytecodeHash, bytes32 cType, uint256 ver) internal { + // QUESTION: what if bytecodeHash is non-zero but different from already set? + if (_allowedBytecodeHashes[cType][ver] != 0) return; - // - // DOMAIN MANAGEMENT - // + _allowedBytecodeHashes[cType][ver] = bytecodeHash; + emit AllowBytecode(bytecodeHash, cType, ver); - /// @notice Adds a new public domain - /// @param domain Domain identifier to add - /// @dev Non-revertable to avoid blocking InstanceManager - function addPublicDomain(bytes32 domain) external onlyOwner { - if (domain == bytes32(0)) { - return; - } + _updateVersionInfo(cType, ver); + } - if (LibString.fromSmallString(domain).contains("::")) { - return; - } + // ------------------------- // + // PUBLIC DOMAINS MANAGEMENT // + // ------------------------- // - if (_publicDomains.add(domain)) { - emit AddPublicDomain(domain); - } + /// @notice Whether `cType` belongs to a public domain + function isInPublicDomain(bytes32 cType) public view override returns (bool) { + return _publicDomainsSet.contains(cType.extractDomain()); } - /// @notice Removes a public domain - /// @param domain Domain identifier to remove - function removePublicDomain(bytes32 domain) external onlyOwner { - if (_publicDomains.remove(domain)) { - emit RemovePublicDomain(domain); - } + /// @notice Whether `domain` is in the list of public domains + function isPublicDomain(bytes32 domain) external view override returns (bool) { + return _publicDomainsSet.contains(domain); } - /// @notice Marks initCode as forbidden - /// @param initCodeHash Hash of initCode to forbid - function forbidInitCode(bytes32 initCodeHash) external onlyOwner { - forbiddenInitCode[initCodeHash] = true; - emit ForbidBytecode(initCodeHash); + /// @notice Returns list of all public domains + function getPublicDomains() external view override returns (bytes32[] memory) { + return _publicDomainsSet.values(); } - /// @notice Sets token-specific postfix - /// @param token Token address - /// @param postfix Postfix to associate with token - function setTokenSpecificPostfix(address token, bytes32 postfix) external onlyOwner { - tokenSpecificPostfixes[token] = postfix; - emit SetTokenSpecificPostfix(token, postfix); + /// @notice Adds `domain` to the list of public domains (does nothing if `domain` contains "::") + /// @dev Can only be called by the owner + function addPublicDomain(bytes32 domain) external override onlyOwner { + if (domain == bytes32(0) || LibString.fromSmallString(domain).contains("::")) return; + if (_publicDomainsSet.add(domain)) emit AddPublicDomain(domain); } - /// @notice Removes contract type owner - /// @param _contractType Contract type to remove owner from - /// @dev Used to remove malicious auditors and cybersquatters - function removeContractTypeOwner(bytes32 _contractType) external onlyOwner { - if (contractTypeOwner[_contractType] != address(0)) { - contractTypeOwner[_contractType] = address(0); - emit RemoveContractTypeOwner(_contractType); - } + /// @notice Removes `domain` from the list of public domains + /// @dev Can only be called by the owner + function removePublicDomain(bytes32 domain) external override onlyOwner { + if (_publicDomainsSet.remove(domain)) emit RemovePublicDomain(domain); } - function revokeApproval(bytes32 _contractType, uint256 _version, bytes32 _bytecodeHash) external onlyOwner { - if (approvedBytecodeHash[_contractType][_version] == _bytecodeHash) { - approvedBytecodeHash[_contractType][_version] = bytes32(0); - emit RevokeApproval(_bytecodeHash, _contractType, _version); - } - } + // ------------------- // + // AUDITORS MANAGEMENT // + // ------------------- // - // GETTERS + /// @notice Whether `auditor` is an approved auditor + function isAuditor(address auditor) public view override returns (bool) { + return _auditorsSet.contains(auditor); + } - /// @notice Checks if a contract name belongs to public domain - /// @param _contractType Contract type to check - /// @return bool True if contract is in public domain - function isInPublicDomain(bytes32 _contractType) public view returns (bool) { - string memory contractTypeStr = _contractType.fromSmallString(); - return isPublicDomain(contractTypeStr.extractDomain().toSmallString()); + /// @notice Returns list of all approved auditors + function getAuditors() external view override returns (address[] memory) { + return _auditorsSet.values(); } - /// @notice Checks if a domain is public - /// @param domain Domain to check - /// @return bool True if domain is public - function isPublicDomain(bytes32 domain) public view returns (bool) { - return _publicDomains.contains(domain); + /// @notice Returns `auditor`'s name + function getAuditorName(address auditor) external view override returns (string memory) { + return _auditorNames[auditor]; } - /// @notice Returns list of all public domains - /// @return Array of public domain identifiers - function listPublicDomains() external view returns (bytes32[] memory) { - return _publicDomains.values(); + /// @notice Adds `auditor` to the list of approved auditors + /// @dev Can only be called by the owner + /// @dev Reverts if `auditor` is zero address + function addAuditor(address auditor, string memory name) external override onlyOwner nonZeroAddress(auditor) { + if (!_auditorsSet.add(auditor)) return; + _auditorNames[auditor] = name; + emit AddAuditor(auditor, name); } - /// @notice Gets token-specific postfix - /// @param token Token address to query - /// @return bytes32 Postfix associated with token - function getTokenSpecificPostfix(address token) external view returns (bytes32) { - return tokenSpecificPostfixes[token]; + /// @notice Removes `auditor` from the list of approved auditors + /// @dev Can only be called by the owner + function removeAuditor(address auditor) external override onlyOwner { + if (!_auditorsSet.remove(auditor)) return; + delete _auditorNames[auditor]; + emit RemoveAuditor(auditor); } - /// @notice Gets latest version for a contract type - /// @param _contractType Contract type to query - /// @return uint256 Latest version number (0 if none exists) - function getLatestVersion(bytes32 _contractType) external view returns (uint256) { - return latestVersion[_contractType]; + // ------------------- // + // FORBIDDING INITCODE // + // ------------------- // + + /// @notice Whether init code with `initCodeHash` is forbidden + function isInitCodeForbidden(bytes32 initCodeHash) external view override returns (bool) { + return _isInitCodeForbidden[initCodeHash]; } - /// @notice Gets latest minor version for a major version - /// @param _contractType Contract type to query - /// @param majorVersion Major version number - /// @return uint256 Latest minor version number - function getLatestMinorVersion(bytes32 _contractType, uint256 majorVersion) external view returns (uint256) { - majorVersion -= majorVersion % 100; - return latestMinorVersion[_contractType][majorVersion]; + /// @notice Marks init code with `initCodeHash` as forbidden + /// @dev Can only be called by the owner + function forbidInitCode(bytes32 initCodeHash) external override onlyOwner { + if (_isInitCodeForbidden[initCodeHash]) return; + _isInitCodeForbidden[initCodeHash] = true; + emit ForbidInitCode(initCodeHash); } - /// @notice Gets latest patch version for a minor version - /// @param _contractType Contract type to query - /// @param minorVersion Minor version number - /// @return uint256 Latest patch version number - function getLatestPatchVersion(bytes32 _contractType, uint256 minorVersion) external view returns (uint256) { - minorVersion -= minorVersion % 10; - return latestPatchVersion[_contractType][minorVersion]; + /// @dev Reverts if `initCode` is forbidden + function _revertIfInitCodeIsForbidden(bytes memory initCode) internal view { + bytes32 initCodeHash = keccak256(initCode); + if (_isInitCodeForbidden[initCodeHash]) revert InitCodeIsForbiddenException(initCodeHash); } - function auditorSignaturesByHash(bytes32 bytecodeHash) external view returns (AuditorSignature[] memory) { - return _auditorSignaturesByHash[bytecodeHash]; + // ------------------------ // + // TOKENS WITH CUSTOM LOGIC // + // ------------------------ // + + /// @notice Returns `token`'s specific postfix, if any + function getTokenSpecificPostfix(address token) external view override returns (bytes32) { + return _tokenSpecificPostfixes[token]; } - function auditorSignaturesByHash(bytes32 bytecodeHash, uint256 index) - external - view - returns (AuditorSignature memory) - { - return _auditorSignaturesByHash[bytecodeHash][index]; + /// @notice Sets `token`'s specific `postfix` (does nothing if `postfix` contains "::") + /// @dev Can only be called by the owner + function setTokenSpecificPostfix(address token, bytes32 postfix) external override onlyOwner { + if (_tokenSpecificPostfixes[token] == postfix || LibString.fromSmallString(postfix).contains("::")) return; + _tokenSpecificPostfixes[token] = postfix; + emit SetTokenSpecificPostfix(token, postfix); } - // - // HELPERS - // - function isBytecodeUploaded(bytes32 bytecodeHash) public view returns (bool) { - return _bytecodeByHash[bytecodeHash].author != address(0); + // --------------- // + // VERSION CONTROL // + // --------------- // + + /// @notice Returns the latest known bytecode version for given `cType` + /// @dev Reverts if `cType` has no bytecode entries + function getLatestVersion(bytes32 cType) external view override returns (uint256 ver) { + ver = _versionInfo[cType].latest; + if (ver == 0) revert VersionNotFoundException(cType); } - function revertIfInitCodeForbidden(bytes memory initCode) public view { - bytes32 initCodeHash = keccak256(initCode); - if (forbiddenInitCode[initCodeHash]) { - revert BytecodeForbiddenException(initCodeHash); - } + /// @notice Returns the latest known minor version for given `cType` and `majorVersion` + /// @dev Reverts if `majorVersion` is less than `100` + /// @dev Reverts if `cType` has no bytecode entries with matching `majorVersion` + function getLatestMinorVersion(bytes32 cType, uint256 majorVersion) external view override returns (uint256 ver) { + _validateVersion(cType, majorVersion); + ver = _versionInfo[cType].latestByMajor[_getMajorVersion(majorVersion)]; + if (ver == 0) revert VersionNotFoundException(cType); } - function isAuditBytecode(bytes32 bytecodeHash) public view returns (bool) { - uint256 len = _auditorSignaturesByHash[bytecodeHash].length; + /// @notice Returns the latest known patch version for given `cType` and `minorVersion` + /// @dev Reverts if `minorVersion` is less than `100` + /// @dev Reverts if `cType` has no bytecode entries with matching `minorVersion` + function getLatestPatchVersion(bytes32 cType, uint256 minorVersion) external view override returns (uint256 ver) { + _validateVersion(cType, minorVersion); + ver = _versionInfo[cType].latestByMinor[_getMinorVersion(minorVersion)]; + if (ver == 0) revert VersionNotFoundException(cType); + } - for (uint256 i = 0; i < len; ++i) { - AuditorSignature memory sig = _auditorSignaturesByHash[bytecodeHash][i]; - if (isAuditor(sig.auditor)) { - return true; - } - } + /// @dev Updates version info for `cType` based on `ver` + function _updateVersionInfo(bytes32 cType, uint256 ver) internal { + VersionInfo storage info = _versionInfo[cType]; + if (ver > info.latest) info.latest = ver; + uint256 majorVersion = _getMajorVersion(ver); + if (ver > info.latestByMajor[majorVersion]) info.latestByMajor[majorVersion] = ver; + uint256 minorVersion = _getMinorVersion(ver); + if (ver > info.latestByMinor[minorVersion]) info.latestByMinor[minorVersion] = ver; + } - return false; + /// @dev Returns the major version of a given version + function _getMajorVersion(uint256 ver) internal pure returns (uint256) { + return ver - ver % 100; } - function bytecodeByHash(bytes32 bytecodeHash) external view returns (Bytecode memory) { - BytecodePointer memory _bytecode = _bytecodeByHash[bytecodeHash]; - return Bytecode({ - contractType: _bytecode.contractType, - version: _bytecode.version, - initCode: SSTORE2.read(_bytecode.initCodePointer), - author: _bytecode.author, - source: _bytecode.source, - authorSignature: _bytecode.authorSignature - }); + /// @dev Returns the minor version of a given version + function _getMinorVersion(uint256 ver) internal pure returns (uint256) { + return ver - ver % 10; } - function domainSeparatorV4() external view returns (bytes32) { - return _domainSeparatorV4(); + /// @dev Reverts if `ver` is less than `100` + function _validateVersion(bytes32 key, uint256 ver) internal pure { + if (ver < 100) revert InvalidVersionException(key, ver); } } diff --git a/contracts/instance/PriceFeedStore.sol b/contracts/instance/PriceFeedStore.sol index c9d352e..2ee31d8 100644 --- a/contracts/instance/PriceFeedStore.sol +++ b/contracts/instance/PriceFeedStore.sol @@ -293,7 +293,7 @@ contract PriceFeedStore is /// @dev Returns whether `priceFeed` is deployed externally or via BCR. /// For latter case, also ensures that price feed is owned by the store. function _validatePriceFeedDeployment(address priceFeed) internal view returns (bool) { - if (IBytecodeRepository(bytecodeRepository).deployedContracts(priceFeed) == 0) return true; + if (!IBytecodeRepository(bytecodeRepository).isDeployedFromRepository(priceFeed)) return true; try Ownable(priceFeed).owner() returns (address owner_) { if (owner_ != address(this)) revert PriceFeedIsNotOwnedByStore(priceFeed); diff --git a/contracts/interfaces/IBytecodeRepository.sol b/contracts/interfaces/IBytecodeRepository.sol index 6449c97..4e0096d 100644 --- a/contracts/interfaces/IBytecodeRepository.sol +++ b/contracts/interfaces/IBytecodeRepository.sol @@ -5,230 +5,152 @@ pragma solidity ^0.8.23; import {IVersion} from "@gearbox-protocol/core-v3/contracts/interfaces/base/IVersion.sol"; import {IImmutableOwnableTrait} from "./base/IImmutableOwnableTrait.sol"; -import {Bytecode, AuditorSignature} from "./Types.sol"; +import {AuditReport, Bytecode} from "./Types.sol"; +/// @title Bytecode repository interface interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { - // - // ERRORS - // - error BytecodeIsNotApprovedException(bytes32 contractType, uint256 version); + // ------ // + // EVENTS // + // ------ // - // Thrown if the deployed contract has a different contractType/version than it's indexed in the repository - error IncorrectBytecodeException(bytes32 bytecodeHash); - - // Thrown if the bytecode provided is empty - error EmptyBytecodeException(); - - // Thrown if someone tries to deploy the contract with the same address - error BytecodeAlreadyExistsAtAddressException(address); - - // Thrown if domain + postfix length is more than 30 symbols (doesn't fit into bytes32) - error TooLongContractTypeException(string); - - // Thrown if requested bytecode wasn't found in the repository - error BytecodeIsNotUploadedException(bytes32 bytecodeHash); - - // Thrown if someone tries to replace existing bytecode with the same contact type & version - error BytecodeAlreadyExistsException(); - - // Thrown if requested bytecode wasn't found in the repository - error BytecodeIsNotAuditedException(); - - // Thrown if someone tries to deploy a contract which wasn't audited enough - error ContractIsNotAuditedException(); - - error SignerIsNotAuditorException(address signer); - - // Thrown when an attempt is made to add an auditor that already exists - error AuditorAlreadyAddedException(); - - // Thrown when an auditor is not found in the repository - error AuditorNotFoundException(); - - // Thrown if the caller is not the deployer of the bytecode - error NotDeployerException(); - - // Thrown if the caller does not have valid auditor permissions - error NoValidAuditorPermissionsAException(); - - /// @notice Thrown when trying to deploy contract with forbidden bytecode - error BytecodeForbiddenException(bytes32 bytecodeHash); - - /// @notice Thrown when trying to deploy contract with incorrect domain ownership - error NotDomainOwnerException(); - - /// @notice Thrown when trying to deploy contract with incorrect domain ownership - error NotAllowedSystemContractException(bytes32 bytecodeHash); - - /// @notice Thrown when trying to deploy contract with incorrect contract type - error ContractTypeVersionAlreadyExistsException(); - - error OnlyAuthorCanSyncException(); - - error AuditorAlreadySignedException(); - - error NoValidAuditorSignatureException(); - - error InvalidAuthorSignatureException(); - // - // EVENTS - // - - // Emitted when new smart contract was deployed + event AddAuditor(address indexed auditor, string name); + event AddPublicDomain(bytes32 indexed domain); + event AuditBytecode(bytes32 indexed bytecodeHash, address indexed auditor, string reportUrl); event DeployContract( - address indexed addr, bytes32 indexed bytecodeHash, string contractType, uint256 indexed version + bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address contractAddress ); - - // Event emitted when a new auditor is added to the repository - event AddAuditor(address indexed auditor, string name); - - // Event emitted when an auditor is forbidden from the repository + event ForbidInitCode(bytes32 indexed initCodeHash); event RemoveAuditor(address indexed auditor); - - // Event emitted when new bytecode is uploaded to the repository - event UploadBytecode( - bytes32 indexed bytecodeHash, - string contractType, - uint256 indexed version, - address indexed author, - string source - ); - - // Event emitted when bytecode is signed by an auditor - event AuditBytecode(bytes32 indexed bytecodeHash, address indexed auditor, string reportUrl, bytes signature); - - // Event emitted when a public domain is added - event AddPublicDomain(bytes32 indexed domain); - - // Event emitted when a public domain is removed event RemovePublicDomain(bytes32 indexed domain); - - // Event emitted when contract type owner is removed - event RemoveContractTypeOwner(bytes32 indexed contractType); - - // Event emitted when bytecode is forbidden - event ForbidBytecode(bytes32 indexed bytecodeHash); - - // Event emitted when token specific postfix is set event SetTokenSpecificPostfix(address indexed token, bytes32 indexed postfix); + event UploadBytecode( + bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address author, string source + ); - // Event emitted when bytecode is approved - event ApproveContract(bytes32 indexed bytecodeHash, bytes32 indexed contractType, uint256 version); - - // Event emitted when bytecode is revoked - event RevokeApproval(bytes32 indexed bytecodeHash, bytes32 indexed contractType, uint256 version); + // ------ // + // ERRORS // + // ------ // - // FUNCTIONS + error AuditorIsNotApprovedException(address auditor); + error AuthorIsNotContractTypeOwnerException(bytes32 cType, address author); + error BytecodeIsAlreadyAllowedException(bytes32 cType, uint256 ver); + error BytecodeIsAlreadySignedByAuditorException(bytes32 bytecodeHash, address auditor); + error BytecodeIsNotAllowedException(bytes32 cType, uint256 ver); + error BytecodeIsNotUploadedException(bytes32 bytecodeHash); + error CallerIsNotBytecodeAuthorException(address caller); + error ContractIsAlreadyDeployedException(address deployedContract); + error InitCodeIsForbiddenException(bytes32 initCodeHash); + error InvalidAuditorSignatureException(address auditor); + error InvalidAuthorSignatureException(address author); + error InvalidBytecodeException(bytes32 bytecodeHash); + error InvalidVersionException(bytes32 cType, uint256 ver); + error VersionNotFoundException(bytes32 cType); + + // --------------- // + // EIP-712 GETTERS // + // --------------- // - function deploy(bytes32 type_, uint256 version_, bytes memory constructorParams, bytes32 salt) + function BYTECODE_TYPEHASH() external view returns (bytes32); + function AUDIT_REPORT_TYPEHASH() external view returns (bytes32); + function domainSeparatorV4() external view returns (bytes32); + function computeBytecodeHash(Bytecode calldata bytecode) external view returns (bytes32); + function computeAuditReportHash(bytes32 bytecodeHash, address auditor, string calldata reportUrl) external - returns (address); + view + returns (bytes32); + + // ------------------- // + // DEPLOYING CONTRACTS // + // ------------------- // + function isDeployedFromRepository(address deployedContract) external view returns (bool); + function getDeployedContractBytecodeHash(address deployedContract) external view returns (bytes32); function computeAddress( - bytes32 type_, - uint256 version_, - bytes memory constructorParams, + bytes32 cType, + uint256 ver, + bytes calldata constructorParams, bytes32 salt, address deployer ) external view returns (address); + function deploy(bytes32 cType, uint256 ver, bytes calldata constructorParams, bytes32 salt) + external + returns (address); - function getTokenSpecificPostfix(address token) external view returns (bytes32); + // ------------------ // + // UPLOADING BYTECODE // + // ------------------ // - function getLatestVersion(bytes32 type_) external view returns (uint256); + function getBytecode(bytes32 bytecodeHash) external view returns (Bytecode memory); + function isBytecodeUploaded(bytes32 bytecodeHash) external view returns (bool); + function uploadBytecode(Bytecode calldata bytecode) external; - function getLatestMinorVersion(bytes32 type_, uint256 majorVersion) external view returns (uint256); + // ----------------- // + // AUDITING BYTECODE // + // ----------------- // - function getLatestPatchVersion(bytes32 type_, uint256 minorVersion) external view returns (uint256); + function isBytecodeAudited(bytes32 bytecodeHash) external view returns (bool); + function getAuditReports(bytes32 bytecodeHash) external view returns (AuditReport[] memory); + function getAuditReport(bytes32 bytecodeHash, uint256 index) external view returns (AuditReport memory); + function getNumAuditReports(bytes32 bytecodeHash) external view returns (uint256); + function submitAuditReport(bytes32 bytecodeHash, AuditReport calldata auditReport) external; - /// @notice Computes a unique hash for bytecode metadata - function computeBytecodeHash(Bytecode calldata bytecode) external pure returns (bytes32); + // ----------------- // + // ALLOWING BYTECODE // + // ----------------- // - /// @notice Uploads new bytecode to the repository - function uploadBytecode(Bytecode calldata bytecode) external; + // TODO: rework this section - /// @notice Allows auditors to sign bytecode metadata - function signBytecodeHash(bytes32 bytecodeHash, string calldata reportUrl, bytes memory signature) external; + event AllowBytecode(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); + event ForbidBytecode(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); + event SetContractTypeOwner(bytes32 indexed cType, address indexed owner); + event RemoveContractTypeOwner(bytes32 indexed cType); - /// @notice Allows owner to mark contracts as system contracts + function getAllowedBytecodeHash(bytes32 cType, uint256 ver) external view returns (bytes32); + function isAllowedSystemContract(bytes32 bytecodeHash) external view returns (bool); + function getContractTypeOwner(bytes32 cType) external view returns (address); function allowSystemContract(bytes32 bytecodeHash) external; + function revokeApproval(bytes32 cType, uint256 ver, bytes32 bytecodeHash) external; + function removeContractTypeOwner(bytes32 cType) external; - /// @notice Adds a new auditor - function addAuditor(address auditor, string memory name) external; - - /// @notice Removes an auditor - function removeAuditor(address auditor) external; + // ------------------------- // + // PUBLIC DOMAINS MANAGEMENT // + // ------------------------- // - /// @notice Checks if an address is an approved auditor - function isAuditor(address auditor) external view returns (bool); - - /// @notice Returns list of all approved auditors - function getAuditors() external view returns (address[] memory); - - /// @notice Adds a new public domain + function isInPublicDomain(bytes32 cType) external view returns (bool); + function isPublicDomain(bytes32 domain) external view returns (bool); + function getPublicDomains() external view returns (bytes32[] memory); function addPublicDomain(bytes32 domain) external; - - /// @notice Removes a public domain function removePublicDomain(bytes32 domain) external; - /// @notice Marks initCode as forbidden - function forbidInitCode(bytes32 initCodeHash) external; - - /// @notice Sets token-specific postfix - function setTokenSpecificPostfix(address token, bytes32 postfix) external; - - /// @notice Removes contract type owner - function removeContractTypeOwner(bytes32 contractType) external; + // ------------------- // + // AUDITORS MANAGEMENT // + // ------------------- // - /// @notice Revokes approval for a specific bytecode - function revokeApproval(bytes32 contractType, uint256 version, bytes32 bytecodeHash) external; - - /// @notice Checks if a contract name belongs to public domain - function isInPublicDomain(bytes32 contractType) external view returns (bool); - - /// @notice Checks if a domain is public - function isPublicDomain(bytes32 domain) external view returns (bool); - - /// @notice Returns list of all public domains - function listPublicDomains() external view returns (bytes32[] memory); - - /// @notice Gets bytecode metadata by hash - function bytecodeByHash(bytes32 hash) external view returns (Bytecode memory); - - /// @notice Gets approved bytecode hash for contract type and version - function approvedBytecodeHash(bytes32 contractType, uint256 version) external view returns (bytes32); - - /// @notice Gets deployed contract's bytecode hash - function deployedContracts(address contractAddress) external view returns (bytes32); - - /// @notice Checks if initCode is forbidden - function forbiddenInitCode(bytes32 initCodeHash) external view returns (bool); - - /// @notice Checks if contract is allowed as system contract - function allowedSystemContracts(bytes32 bytecodeHash) external view returns (bool); - - /// @notice Gets contract type owner - function contractTypeOwner(bytes32 contractType) external view returns (address); - - /// @notice Gets auditor name - function auditorName(address auditor) external view returns (string memory); + function isAuditor(address auditor) external view returns (bool); + function getAuditors() external view returns (address[] memory); + function getAuditorName(address auditor) external view returns (string memory); + function addAuditor(address auditor, string calldata name) external; + function removeAuditor(address auditor) external; - /// @notice Gets auditor signatures for a bytecode hash - function auditorSignaturesByHash(bytes32 bytecodeHash) external view returns (AuditorSignature[] memory); + // ------------------- // + // FORBIDDING INITCODE // + // ------------------- // - /// @notice Gets specific auditor signature for a bytecode hash - function auditorSignaturesByHash(bytes32 bytecodeHash, uint256 index) - external - view - returns (AuditorSignature memory); + function isInitCodeForbidden(bytes32 initCodeHash) external view returns (bool); + function forbidInitCode(bytes32 initCodeHash) external; - /// @notice Checks if bytecode is uploaded - function isBytecodeUploaded(bytes32 bytecodeHash) external view returns (bool); + // ------------------------ // + // TOKENS WITH CUSTOM LOGIC // + // ------------------------ // - /// @notice Checks if initCode is forbidden and reverts if it is - function revertIfInitCodeForbidden(bytes memory initCode) external view; + function getTokenSpecificPostfix(address token) external view returns (bytes32); + function setTokenSpecificPostfix(address token, bytes32 postfix) external; - /// @notice Checks if bytecode is audited - function isAuditBytecode(bytes32 bytecodeHash) external view returns (bool); + // --------------- // + // VERSION CONTROL // + // --------------- // - function BYTECODE_TYPEHASH() external view returns (bytes32); + function getLatestVersion(bytes32 cType) external view returns (uint256); + function getLatestMinorVersion(bytes32 cType, uint256 majorVersion) external view returns (uint256); + function getLatestPatchVersion(bytes32 cType, uint256 minorVersion) external view returns (uint256); } diff --git a/contracts/interfaces/Types.sol b/contracts/interfaces/Types.sol index 83fb7f2..4c3514b 100644 --- a/contracts/interfaces/Types.sol +++ b/contracts/interfaces/Types.sol @@ -9,29 +9,46 @@ struct AddressProviderEntry { address value; } +struct AuditReport { + address auditor; + string reportUrl; + bytes signature; +} + +struct Bytecode { + bytes32 contractType; + uint256 version; + bytes initCode; + address author; + string source; + bytes authorSignature; +} + +struct BytecodePointer { + bytes32 contractType; + uint256 version; + address initCodePointer; + address author; + string source; + bytes authorSignature; +} + struct Call { address target; bytes callData; } +struct ConnectedPriceFeed { + address token; + address[] priceFeeds; +} + struct CrossChainCall { uint256 chainId; // 0 means to be executed on all chains address target; bytes callData; } -struct SignedBatch { - string name; - bytes32 prevHash; - CrossChainCall[] calls; - bytes[] signatures; -} - -struct SignedRecoveryModeMessage { - bytes32 startingBatchHash; - bytes[] signatures; -} - struct DeployParams { bytes32 postfix; bytes32 salt; @@ -58,33 +75,16 @@ struct PriceFeedInfo { uint256 version; } -struct ConnectedPriceFeed { - address token; - address[] priceFeeds; -} - -struct Bytecode { - bytes32 contractType; - uint256 version; - bytes initCode; - address author; - string source; - bytes authorSignature; -} - -struct BytecodePointer { - bytes32 contractType; - uint256 version; - address initCodePointer; - address author; - string source; - bytes authorSignature; +struct SignedBatch { + string name; + bytes32 prevHash; + CrossChainCall[] calls; + bytes[] signatures; } -struct AuditorSignature { - string reportUrl; - address auditor; - bytes signature; +struct SignedRecoveryModeMessage { + bytes32 startingBatchHash; + bytes[] signatures; } struct Split { diff --git a/contracts/market/MarketConfigurator.sol b/contracts/market/MarketConfigurator.sol index e636d68..3c01094 100644 --- a/contracts/market/MarketConfigurator.sol +++ b/contracts/market/MarketConfigurator.sol @@ -899,7 +899,7 @@ contract MarketConfigurator is DeployerTrait, IMarketConfigurator { /// @dev Reverts if caller is not the admin /// @dev Reverts if contract is not deployed via bytecode repository function addPeripheryContract(address peripheryContract) external override onlyAdmin { - if (IBytecodeRepository(bytecodeRepository).deployedContracts(peripheryContract) == 0) { + if (!IBytecodeRepository(bytecodeRepository).isDeployedFromRepository(peripheryContract)) { revert IncorrectPeripheryContractException(peripheryContract); } bytes32 domain = _getDomain(peripheryContract); diff --git a/contracts/test/configuration/MarketConfigurator.unit.t.sol b/contracts/test/configuration/MarketConfigurator.unit.t.sol index 309d8d0..da6c7d9 100644 --- a/contracts/test/configuration/MarketConfigurator.unit.t.sol +++ b/contracts/test/configuration/MarketConfigurator.unit.t.sol @@ -328,7 +328,9 @@ contract MarketConfiguratorUnitTest is ConfigurationTestHelper { // Mock bytecode repository to recognize the contract vm.mockCall( - bytecodeRepository, abi.encodeWithSignature("deployedContracts(address)", peripheryContract), abi.encode(1) + bytecodeRepository, + abi.encodeWithSignature("isDeployedFromRepository(address)", peripheryContract), + abi.encode(true) ); // Test that only admin can add periphery contracts @@ -372,8 +374,11 @@ contract MarketConfiguratorUnitTest is ConfigurationTestHelper { // Test adding contract that's not in bytecode repository vm.mockCall( - bytecodeRepository, abi.encodeWithSignature("deployedContracts(address)", peripheryContract), abi.encode(0) + bytecodeRepository, + abi.encodeWithSignature("isDeployedFromRepository(address)", peripheryContract), + abi.encode(false) ); + vm.prank(admin); vm.expectRevert( abi.encodeWithSelector(IMarketConfigurator.IncorrectPeripheryContractException.selector, peripheryContract) @@ -383,7 +388,9 @@ contract MarketConfiguratorUnitTest is ConfigurationTestHelper { // Test adding contract that doesn't implement IVersion address invalidContract = address(new GeneralMock()); vm.mockCall( - bytecodeRepository, abi.encodeWithSignature("deployedContracts(address)", invalidContract), abi.encode(1) + bytecodeRepository, + abi.encodeWithSignature("isDeployedFromRepository(address)", peripheryContract), + abi.encode(true) ); vm.prank(admin); vm.expectRevert( diff --git a/contracts/test/global/BytecodeRepository.unit.t.sol b/contracts/test/global/BytecodeRepository.unit.t.sol index e75e234..c831903 100644 --- a/contracts/test/global/BytecodeRepository.unit.t.sol +++ b/contracts/test/global/BytecodeRepository.unit.t.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.23; import {Test} from "forge-std/Test.sol"; import {BytecodeRepository} from "../../global/BytecodeRepository.sol"; import {IBytecodeRepository} from "../../interfaces/IBytecodeRepository.sol"; -import {Bytecode, AuditorSignature} from "../../interfaces/Types.sol"; +import {AuditReport, Bytecode} from "../../interfaces/Types.sol"; import {LibString} from "@solady/utils/LibString.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; @@ -93,7 +93,7 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { // Verify bytecode was stored assertTrue(repository.isBytecodeUploaded(bytecodeHash)); - Bytecode memory storedBc = repository.bytecodeByHash(bytecodeHash); + Bytecode memory storedBc = repository.getBytecode(bytecodeHash); assertEq(storedBc.contractType, _TEST_CONTRACT); assertEq(storedBc.version, _TEST_VERSION); assertEq(storedBc.author, author); @@ -120,9 +120,6 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { vm.startPrank(author); repository.uploadBytecode(bc); - - vm.expectRevert(IBytecodeRepository.BytecodeAlreadyExistsException.selector); - repository.uploadBytecode(bc); vm.stopPrank(); } @@ -145,40 +142,46 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { /// AUDITOR SIGNATURE TESTS - function test_BCR_04_signBytecodeHash_works() public { + function test_BCR_04_submitAuditReport_works() public { // First upload bytecode bytes32 bytecodeHash = _uploadTestBytecode(); // Now sign as auditor string memory reportUrl = "https://audit.report"; bytes32 signatureHash = repository.domainSeparatorV4().toTypedDataHash( - keccak256(abi.encode(repository.AUDITOR_SIGNATURE_TYPEHASH(), bytecodeHash, keccak256(bytes(reportUrl)))) + keccak256( + abi.encode(repository.AUDIT_REPORT_TYPEHASH(), bytecodeHash, auditor, keccak256(bytes(reportUrl))) + ) ); (uint8 v, bytes32 r, bytes32 s) = vm.sign(auditorPK, signatureHash); bytes memory signature = abi.encodePacked(r, s, v); + AuditReport memory auditReport = AuditReport({auditor: auditor, reportUrl: reportUrl, signature: signature}); + vm.prank(auditor); - repository.signBytecodeHash(bytecodeHash, reportUrl, signature); + repository.submitAuditReport(bytecodeHash, auditReport); // Verify signature was stored - assertTrue(repository.isAuditBytecode(bytecodeHash)); + assertTrue(repository.isBytecodeAudited(bytecodeHash)); - AuditorSignature[] memory sigs = repository.auditorSignaturesByHash(bytecodeHash); - assertEq(sigs.length, 1); - assertEq(sigs[0].auditor, auditor); - assertEq(sigs[0].reportUrl, reportUrl); - assertEq(sigs[0].signature, signature); + AuditReport[] memory reports = repository.getAuditReports(bytecodeHash); + assertEq(reports.length, 1); + assertEq(reports[0].auditor, auditor); + assertEq(reports[0].reportUrl, reportUrl); + assertEq(reports[0].signature, signature); } - function test_BCR_05_signBytecodeHash_reverts_if_not_auditor() public { + function test_BCR_05_submitAuditReport_reverts_if_not_auditor() public { // First upload bytecode bytes32 bytecodeHash = _uploadTestBytecode(); // Now sign as auditor string memory reportUrl = "https://audit.report"; bytes32 signatureHash = repository.domainSeparatorV4().toTypedDataHash( - keccak256(abi.encode(repository.AUDITOR_SIGNATURE_TYPEHASH(), bytecodeHash, keccak256(bytes(reportUrl)))) + keccak256( + abi.encode(repository.AUDIT_REPORT_TYPEHASH(), bytecodeHash, auditor, keccak256(bytes(reportUrl))) + ) ); uint256 notAuditorPK = vm.randomUint(); @@ -187,9 +190,11 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { (uint8 v, bytes32 r, bytes32 s) = vm.sign(notAuditorPK, signatureHash); bytes memory signature = abi.encodePacked(r, s, v); + AuditReport memory auditReport = AuditReport({auditor: notAuditor, reportUrl: reportUrl, signature: signature}); + vm.prank(notAuditor); - vm.expectRevert(abi.encodeWithSelector(IBytecodeRepository.SignerIsNotAuditorException.selector, notAuditor)); - repository.signBytecodeHash(bytecodeHash, reportUrl, signature); + vm.expectRevert(abi.encodeWithSelector(IBytecodeRepository.AuditorIsNotApprovedException.selector, notAuditor)); + repository.submitAuditReport(bytecodeHash, auditReport); } /// DEPLOYMENT TESTS @@ -201,14 +206,18 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { // Now sign as auditor string memory reportUrl = "https://audit.report"; bytes32 signatureHash = repository.domainSeparatorV4().toTypedDataHash( - keccak256(abi.encode(repository.AUDITOR_SIGNATURE_TYPEHASH(), bytecodeHash, keccak256(bytes(reportUrl)))) + keccak256( + abi.encode(repository.AUDIT_REPORT_TYPEHASH(), bytecodeHash, auditor, keccak256(bytes(reportUrl))) + ) ); (uint8 v, bytes32 r, bytes32 s) = vm.sign(auditorPK, signatureHash); bytes memory signature = abi.encodePacked(r, s, v); + AuditReport memory auditReport = AuditReport({auditor: auditor, reportUrl: reportUrl, signature: signature}); + vm.prank(auditor); - repository.signBytecodeHash(bytecodeHash, reportUrl, signature); + repository.submitAuditReport(bytecodeHash, auditReport); // Mark as system contract to auto-approve vm.prank(owner); @@ -221,7 +230,8 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { // Verify deployment assertTrue(deployed.code.length > 0); - assertEq(repository.deployedContracts(deployed), bytecodeHash); + assertTrue(repository.isDeployedFromRepository(deployed)); + assertEq(repository.getDeployedContractBytecodeHash(deployed), bytecodeHash); IVersion version = IVersion(deployed); assertEq(version.contractType(), _TEST_CONTRACT); @@ -231,7 +241,7 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { function test_BCR_07_deploy_reverts_if_not_approved() public { vm.expectRevert( abi.encodeWithSelector( - IBytecodeRepository.BytecodeIsNotApprovedException.selector, _TEST_CONTRACT, _TEST_VERSION + IBytecodeRepository.BytecodeIsNotAllowedException.selector, _TEST_CONTRACT, _TEST_VERSION ) ); repository.deploy(_TEST_CONTRACT, _TEST_VERSION, "", _TEST_SALT); @@ -265,7 +275,7 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { vm.expectRevert( abi.encodeWithSelector( - IBytecodeRepository.BytecodeIsNotApprovedException.selector, _TEST_CONTRACT, _TEST_VERSION + IBytecodeRepository.BytecodeIsNotAllowedException.selector, _TEST_CONTRACT, _TEST_VERSION ) ); repository.deploy(_TEST_CONTRACT, _TEST_VERSION, "", _TEST_SALT); diff --git a/contracts/test/global/PriceFeedStore.unit.t.sol b/contracts/test/global/PriceFeedStore.unit.t.sol index 9e5f38f..650be35 100644 --- a/contracts/test/global/PriceFeedStore.unit.t.sol +++ b/contracts/test/global/PriceFeedStore.unit.t.sol @@ -65,7 +65,9 @@ contract PriceFeedStoreTest is Test { abi.encode(zeroPriceFeed) ); - vm.mockCall(address(bytecodeRepository), abi.encodeWithSignature("deployedContracts(address)"), abi.encode(0)); + vm.mockCall( + address(bytecodeRepository), abi.encodeWithSignature("isDeployedFromRepository(address)"), abi.encode(false) + ); store = new PriceFeedStore(address(addressProvider)); _markAsInternalPriceFeed(address(priceFeed)); @@ -74,8 +76,8 @@ contract PriceFeedStoreTest is Test { function _markAsInternalPriceFeed(address target) internal { vm.mockCall( address(bytecodeRepository), - abi.encodeWithSignature("deployedContracts(address)", target), - abi.encode(keccak256(abi.encodePacked(target))) + abi.encodeWithSignature("isDeployedFromRepository(address)", target), + abi.encode(true) ); vm.mockCall(address(target), abi.encodeWithSignature("owner()"), abi.encode(address(store))); @@ -475,16 +477,16 @@ contract PriceFeedStoreTest is Test { MockPriceFeed nonOwnableFeed = new MockPriceFeed(); vm.mockCall( address(bytecodeRepository), - abi.encodeWithSignature("deployedContracts(address)", address(nonOwnableFeed)), - abi.encode(keccak256(abi.encodePacked(address(nonOwnableFeed)))) + abi.encodeWithSignature("isDeployedFromRepository(address)", address(nonOwnableFeed)), + abi.encode(true) ); // Test Ownable feed owned by store (should pass) MockPriceFeed ownableFeed = new MockPriceFeed(); vm.mockCall( address(bytecodeRepository), - abi.encodeWithSignature("deployedContracts(address)", address(ownableFeed)), - abi.encode(keccak256(abi.encodePacked(address(ownableFeed)))) + abi.encodeWithSignature("isDeployedFromRepository(address)", address(ownableFeed)), + abi.encode(true) ); vm.mockCall(address(ownableFeed), abi.encodeWithSignature("owner()"), abi.encode(address(store))); @@ -492,8 +494,8 @@ contract PriceFeedStoreTest is Test { MockPriceFeed wrongOwnerFeed = new MockPriceFeed(); vm.mockCall( address(bytecodeRepository), - abi.encodeWithSignature("deployedContracts(address)", address(wrongOwnerFeed)), - abi.encode(keccak256(abi.encodePacked(address(wrongOwnerFeed)))) + abi.encodeWithSignature("isDeployedFromRepository(address)", address(wrongOwnerFeed)), + abi.encode(true) ); vm.mockCall(address(wrongOwnerFeed), abi.encodeWithSignature("owner()"), abi.encode(makeAddr("other"))); @@ -501,8 +503,8 @@ contract PriceFeedStoreTest is Test { MockPriceFeed ownable2StepFeed = new MockPriceFeed(); vm.mockCall( address(bytecodeRepository), - abi.encodeWithSignature("deployedContracts(address)", address(ownable2StepFeed)), - abi.encode(keccak256(abi.encodePacked(address(ownable2StepFeed)))) + abi.encodeWithSignature("isDeployedFromRepository(address)", address(ownable2StepFeed)), + abi.encode(true) ); vm.mockCall(address(ownable2StepFeed), abi.encodeWithSignature("owner()"), abi.encode(address(store))); vm.mockCall( diff --git a/contracts/test/helpers/BCRHelpers.sol b/contracts/test/helpers/BCRHelpers.sol index 7387c11..fb31682 100644 --- a/contracts/test/helpers/BCRHelpers.sol +++ b/contracts/test/helpers/BCRHelpers.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.23; import {SignatureHelper} from "./SignatureHelper.sol"; -import {Bytecode} from "../../interfaces/Types.sol"; +import {AuditReport, Bytecode} from "../../interfaces/Types.sol"; import {IBytecodeRepository} from "../../interfaces/IBytecodeRepository.sol"; import {console} from "forge-std/console.sol"; import {LibString} from "@solady/utils/LibString.sol"; @@ -85,8 +85,9 @@ contract BCRHelpers is SignatureHelper { // Build auditor signature bytes32 signatureHash = keccak256( abi.encode( - keccak256("SignBytecodeHash(bytes32 bytecodeHash,string reportUrl)"), + keccak256("AuditReport(bytes32 bytecodeHash,address auditor,string reportUrl)"), bytecodeHash, + auditor, keccak256(bytes(reportUrl)) ) ); @@ -95,8 +96,10 @@ contract BCRHelpers is SignatureHelper { bytes memory signature = _sign(auditorKey, keccak256(abi.encodePacked("\x19\x01", _bytecodeDomainSeparator(), signatureHash))); - // Call signBytecodeHash with signature - IBytecodeRepository(bytecodeRepository).signBytecodeHash(bytecodeHash, reportUrl, signature); + AuditReport memory auditReport = AuditReport({auditor: auditor, reportUrl: reportUrl, signature: signature}); + + // Call submitAuditReport with signature + IBytecodeRepository(bytecodeRepository).submitAuditReport(bytecodeHash, auditReport); } function _bytecodeDomainSeparator() internal view returns (bytes32) { From f80f3fe4fb261fb6246f6e89f68abf36c2e1bbf3 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Thu, 27 Feb 2025 13:42:21 +0200 Subject: [PATCH 2/5] feat: `CrossChainMultisig` cleanup --- contracts/global/CrossChainMultisig.sol | 472 ++++++++++-------- contracts/interfaces/ICrossChainMultisig.sol | 185 +++---- .../test/global/CrossChainMultisig.unit.t.sol | 124 +++-- .../test/global/CrossChainMultisigHarness.sol | 2 +- contracts/test/helpers/CCGHelper.sol | 7 +- 5 files changed, 398 insertions(+), 392 deletions(-) diff --git a/contracts/global/CrossChainMultisig.sol b/contracts/global/CrossChainMultisig.sol index 492e57c..e5d9700 100644 --- a/contracts/global/CrossChainMultisig.sol +++ b/contracts/global/CrossChainMultisig.sol @@ -3,343 +3,385 @@ // (c) Gearbox Foundation, 2024. pragma solidity ^0.8.23; -import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; -import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {LibString} from "@solady/utils/LibString.sol"; -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; -import {SignedBatch, CrossChainCall, SignedRecoveryModeMessage} from "../interfaces/ICrossChainMultisig.sol"; -import {IVersion} from "@gearbox-protocol/core-v3/contracts/interfaces/base/IVersion.sol"; +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {LibString} from "@solady/utils/LibString.sol"; -import {EIP712Mainnet} from "../helpers/EIP712Mainnet.sol"; - -import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; -import {ICrossChainMultisig} from "../interfaces/ICrossChainMultisig.sol"; +import {CrossChainCall, ICrossChainMultisig} from "../interfaces/ICrossChainMultisig.sol"; +import {SignedBatch, SignedRecoveryModeMessage} from "../interfaces/Types.sol"; +import {EIP712Mainnet} from "../helpers/EIP712Mainnet.sol"; import {AP_CROSS_CHAIN_MULTISIG} from "../libraries/ContractLiterals.sol"; +/// @title Cross-chain multisig contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossChainMultisig { + using Address for address; using EnumerableSet for EnumerableSet.AddressSet; using EnumerableSet for EnumerableSet.Bytes32Set; using LibString for bytes32; - using LibString for string; using LibString for uint256; - /// @notice Meta info about contract type & version + /// @notice Contract version uint256 public constant override version = 3_10; + + /// @notice Contract type bytes32 public constant override contractType = AP_CROSS_CHAIN_MULTISIG; - // EIP-712 type hash for Proposal only - bytes32 public constant CROSS_CHAIN_CALL_TYPEHASH = + /// @notice Cross-chain call typehash + bytes32 public constant override CROSS_CHAIN_CALL_TYPEHASH = keccak256("CrossChainCall(uint256 chainId,address target,bytes callData)"); - bytes32 public constant BATCH_TYPEHASH = keccak256("Batch(string name,bytes32 batchHash,bytes32 prevHash)"); - bytes32 public constant RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(bytes32 startingBatchHash)"); - uint8 public confirmationThreshold; + /// @notice Batch typehash + /// @dev This typehash is used to identify batches + bytes32 public constant override BATCH_TYPEHASH = + keccak256("Batch(string name,CrossChainCall[] calls,bytes32 prevHash)"); + + /// @notice Compact batch typehash + /// @dev This typehash is used for signing to avoid cluttering the message with calls + bytes32 public constant override COMPACT_BATCH_TYPEHASH = + keccak256("CompactBatch(string name,bytes32 batchHash,bytes32 prevHash)"); + + /// @notice Recovery mode typehash + bytes32 public constant override RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(bytes32 startingBatchHash)"); + + /// @notice Confirmation threshold + uint8 public override confirmationThreshold; - bytes32 public lastBatchHash; + /// @notice Whether recovery mode is enabled + bool public override isRecoveryModeEnabled = false; - EnumerableSet.AddressSet internal _signers; + /// @dev Set of approved signers + EnumerableSet.AddressSet internal _signersSet; + /// @dev List of executed batch hashes bytes32[] internal _executedBatchHashes; - mapping(bytes32 => EnumerableSet.Bytes32Set) internal _connectedBatchHashes; - mapping(bytes32 => SignedBatch) internal _signedBatches; + /// @dev Mapping from `batchHash` to the set of connected batch hashes + mapping(bytes32 batchHash => EnumerableSet.Bytes32Set) internal _connectedBatchHashes; - bool public recoveryModeEnabled = false; + /// @dev Mapping from `batchHash` to signed batch + mapping(bytes32 batchHash => SignedBatch) internal _signedBatches; + /// @dev Ensures that function can only be called on Ethereum Mainnet modifier onlyOnMainnet() { if (block.chainid != 1) revert CantBeExecutedOnCurrentChainException(); _; } - modifier onlyOnNotMainnet() { + /// @dev Ensures that function can only be called outside Ethereum Mainnet + modifier onlyNotOnMainnet() { if (block.chainid == 1) revert CantBeExecutedOnCurrentChainException(); _; } + /// @dev Ensures that function can only be called by the contract itself modifier onlySelf() { - if (msg.sender != address(this)) revert OnlySelfException(); + if (msg.sender != address(this)) revert CallerIsNotSelfException(msg.sender); _; } - // It's deployed with the same set of parameters on all chains, so it's qddress should be the same - // @param: initialSigners - Array of initial signers - // @param: _confirmationThreshold - Confirmation threshold - // @param: _owner - Owner of the contract. used on Mainnet only, however, it should be same on all chains - // to make CREATE2 address the same on all chains - constructor(address[] memory initialSigners, uint8 _confirmationThreshold, address _owner) + /// @notice Constructor + /// @param signers_ Array of initial signers + /// @param confirmationThreshold_ Confirmation threshold + /// @param owner_ Owner of the contract, assumed to be Gearbox DAO + constructor(address[] memory signers_, uint8 confirmationThreshold_, address owner_) EIP712Mainnet(contractType.fromSmallString(), version.toString()) - Ownable() { - uint256 len = initialSigners.length; + uint256 len = signers_.length; + for (uint256 i; i < len; ++i) { + _addSigner(signers_[i]); + } + _setConfirmationThreshold(confirmationThreshold_); + _transferOwnership(owner_); + } - for (uint256 i = 0; i < len; ++i) { - _addSigner(initialSigners[i]); // U:[SM-1] + // --------------- // + // EIP-712 GETTERS // + // --------------- // + + /// @notice Returns the domain separator + function domainSeparatorV4() external view override returns (bytes32) { + return _domainSeparatorV4(); + } + + /// @notice Computes struct hash for cross-chain call + function computeCrossChainCallHash(CrossChainCall calldata call) public pure override returns (bytes32) { + return keccak256(abi.encode(CROSS_CHAIN_CALL_TYPEHASH, call.chainId, call.target, keccak256(call.callData))); + } + + /// @notice Computes struct hash for batch + function computeBatchHash(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) + public + pure + returns (bytes32) + { + uint256 len = calls.length; + bytes32[] memory callHashes = new bytes32[](len); + for (uint256 i; i < len; ++i) { + callHashes[i] = computeCrossChainCallHash(calls[i]); } + return keccak256( + abi.encode(BATCH_TYPEHASH, keccak256(bytes(name)), keccak256(abi.encodePacked(callHashes)), prevHash) + ); + } - _setConfirmationThreshold(_confirmationThreshold); // U:[SM-1] - _transferOwnership(_owner); // U:[SM-1] + /// @notice Computes struct hash for compact batch + function computeCompactBatchHash(string memory name, bytes32 batchHash, bytes32 prevHash) + public + pure + override + returns (bytes32) + { + return keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes(name)), batchHash, prevHash)); } - // @dev: Submit a new proposal - // Executed by Gearbox DAO on Mainnet - // @param: calls - Array of CrossChainCall structs - // @param: prevHash - Hash of the previous proposal (zero if first proposal) + /// @notice Computes struct hash for recovery mode + function computeRecoveryModeHash(bytes32 startingBatchHash) public pure override returns (bytes32) { + return keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, startingBatchHash)); + } + + // ---------- // + // GOVERNANCE // + // ---------- // + + /// @notice Returns the hash of the last executed batch + function lastBatchHash() public view override returns (bytes32) { + uint256 len = _executedBatchHashes.length; + return len == 0 ? bytes32(0) : _executedBatchHashes[len - 1]; + } + + /// @notice Returns list of executed batch hashes + function getExecutedBatchHashes() external view returns (bytes32[] memory) { + return _executedBatchHashes; + } + + /// @notice Returns list of batch hashes connected to the last executed batch + function getCurrentBatchHashes() external view returns (bytes32[] memory) { + return _connectedBatchHashes[lastBatchHash()].values(); + } + + /// @notice Returns list of batch hashes connected to a batch with `batchHash` + function getConnectedBatchHashes(bytes32 batchHash) external view override returns (bytes32[] memory) { + return _connectedBatchHashes[batchHash].values(); + } + + /// @notice Returns batch by hash + function getBatch(bytes32 batchHash) external view returns (SignedBatch memory result) { + return _signedBatches[batchHash]; + } + + /// @notice Allows Gearbox DAO to submit a new batch on Ethereum Mainnet + /// @dev Can only be executed by Gearbox DAO on Ethereum Mainnet + /// @dev Reverts if `prevHash` is not the hash of the last executed batch + /// @dev Reverts if `calls` is empty or contains local self-calls function submitBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) external + override onlyOwner onlyOnMainnet nonReentrant { - _verifyBatch({calls: calls, prevHash: prevHash}); + bytes32 batchHash = computeBatchHash(name, calls, prevHash); + if (!_connectedBatchHashes[lastBatchHash()].add(batchHash)) return; - bytes32 batchHash = hashBatch({name: name, calls: calls, prevHash: prevHash}); + _verifyBatch(calls, prevHash); - // Copy batch to storage SignedBatch storage signedBatch = _signedBatches[batchHash]; - + signedBatch.name = name; + signedBatch.prevHash = prevHash; uint256 len = calls.length; - for (uint256 i = 0; i < len; ++i) { + for (uint256 i; i < len; ++i) { signedBatch.calls.push(calls[i]); } - signedBatch.prevHash = prevHash; - signedBatch.name = name; - - _connectedBatchHashes[lastBatchHash].add(batchHash); emit SubmitBatch(batchHash); } - // @dev: Sign a proposal - // Executed by any signer to make cross-chain distribution possible - // @param: proposalHash - Hash of the proposal to sign - // @param: signature - Signature of the proposal - function signBatch(bytes32 batchHash, bytes calldata signature) external onlyOnMainnet nonReentrant { + /// @notice Submits a signature for a compact batch message for a batch with `batchHash`. + /// If the number of signatures reaches confirmation threshold, the batch is executed. + /// @dev Can only be executed on Ethereum Mainnet (though permissionlessly to ease signers' life) + /// @dev Reverts if batch with `batchHash` hasn't been submitted or is not connected to the last executed batch + /// @dev Reverts if signer is not approved or their signature has already been submitted + function signBatch(bytes32 batchHash, bytes calldata signature) external override onlyOnMainnet nonReentrant { SignedBatch storage signedBatch = _signedBatches[batchHash]; - if (signedBatch.prevHash != lastBatchHash) { - revert InvalidPrevHashException(); - } - bytes32 digest = _hashTypedDataV4(computeSignBatchHash(signedBatch.name, batchHash, signedBatch.prevHash)); + if (signedBatch.calls.length == 0) revert BatchIsNotSubmittedException(batchHash); + if (signedBatch.prevHash != lastBatchHash()) revert InvalidPrevHashException(); + bytes32 digest = _hashTypedDataV4(computeCompactBatchHash(signedBatch.name, batchHash, signedBatch.prevHash)); address signer = ECDSA.recover(digest, signature); - if (!_signers.contains(signer)) revert SignerDoesNotExistException(); + if (!_signersSet.contains(signer)) revert SignerIsNotApprovedException(signer); signedBatch.signatures.push(signature); - - uint256 validSignatures = _verifySignatures({signatures: signedBatch.signatures, digest: digest}); - emit SignBatch(batchHash, signer); - if (validSignatures >= confirmationThreshold) { - _verifyBatch({calls: signedBatch.calls, prevHash: signedBatch.prevHash}); - _executeBatch({calls: signedBatch.calls, batchHash: batchHash}); - } + uint256 validSignatures = _verifySignatures(signedBatch.signatures, digest); + if (validSignatures >= confirmationThreshold) _executeBatch(signedBatch.calls, batchHash); } - // @dev: Execute a proposal on other chain permissionlessly - function executeBatch(SignedBatch calldata signedBatch) external onlyOnNotMainnet nonReentrant { - bytes32 batchHash = hashBatch(signedBatch.name, signedBatch.calls, signedBatch.prevHash); - - // Check batch is valid - _verifyBatch({calls: signedBatch.calls, prevHash: signedBatch.prevHash}); + /// @notice Executes a proposal outside Ethereum Mainnet permissionlessly + /// @dev In the current implementation, signers are trusted not to deviate and only sign batches + /// submitted by Gearbox DAO on Ethereum Mainnet. In future versions, DAO decisions will be + /// propagated to other chains using bridges or `L1SLOAD`. + /// @dev Reverts if batch's `prevHash` is not the hash of the last executed batch + /// @dev Reverts if batch is empty or contains local self-calls + /// @dev Reverts if signatures have duplicates or the number of valid signatures is insufficient + function executeBatch(SignedBatch calldata signedBatch) external override onlyNotOnMainnet nonReentrant { + _verifyBatch(signedBatch.calls, signedBatch.prevHash); - bytes32 digest = _hashTypedDataV4(computeSignBatchHash(signedBatch.name, batchHash, signedBatch.prevHash)); + bytes32 batchHash = computeBatchHash(signedBatch.name, signedBatch.calls, signedBatch.prevHash); - uint256 validSignatures = _verifySignatures({signatures: signedBatch.signatures, digest: digest}); - if (validSignatures < confirmationThreshold) revert NotEnoughSignaturesException(); + bytes32 digest = _hashTypedDataV4(computeCompactBatchHash(signedBatch.name, batchHash, signedBatch.prevHash)); + uint256 validSignatures = _verifySignatures(signedBatch.signatures, digest); + if (validSignatures < confirmationThreshold) revert InsufficientNumberOfSignaturesException(); - _executeBatch({calls: signedBatch.calls, batchHash: batchHash}); - } - - // @dev: Enable recovery mode - function enableRecoveryMode(SignedRecoveryModeMessage memory message) external onlyOnNotMainnet nonReentrant { - bytes32 digest = _hashTypedDataV4(computeRecoveryModeHash(message.startingBatchHash)); - - if (message.startingBatchHash != lastBatchHash) { - revert InvalidRecoveryModeMessageException(); - } - - uint256 validSignatures = _verifySignatures({signatures: message.signatures, digest: digest}); - if (validSignatures < confirmationThreshold) revert NotEnoughSignaturesException(); - - recoveryModeEnabled = true; - emit EnableRecoveryMode(message.startingBatchHash); + _executeBatch(signedBatch.calls, batchHash); } + /// @dev Ensures that batch is connected to the last executed batch, is non-empty and contains no local self-calls function _verifyBatch(CrossChainCall[] memory calls, bytes32 prevHash) internal view { - if (prevHash != lastBatchHash) revert InvalidPrevHashException(); - if (calls.length == 0) revert NoCallsInProposalException(); + if (prevHash != lastBatchHash()) revert InvalidPrevHashException(); uint256 len = calls.length; - for (uint256 i = 0; i < len; ++i) { - CrossChainCall memory call = calls[i]; - if (call.chainId != 0 && call.target == address(this)) { - revert InconsistentSelfCallOnOtherChainException(); - } - } - } - - function _verifySignatures(bytes[] memory signatures, bytes32 digest) - internal - view - returns (uint256 validSignatures) - { - address[] memory proposalSigners = new address[](signatures.length); - // Check for duplicate signatures - uint256 len = signatures.length; - - for (uint256 i = 0; i < len; ++i) { - address signer = ECDSA.recover(digest, signatures[i]); - - // It's not reverted to avoid the case, when 2 proposals are submitted - // and the first one is about removing a signer. The signer could add his signature - // to the second proposal (it's still possible) and lock the system forever - if (_signers.contains(signer)) { - validSignatures++; - } - for (uint256 j = 0; j < i; ++j) { - if (proposalSigners[j] == signer) { - revert AlreadySignedException(); - } + if (len == 0) revert InvalidBatchException(); + for (uint256 i; i < len; ++i) { + if (calls[i].chainId != 0 && calls[i].target == address(this)) { + revert InvalidBatchException(); } - proposalSigners[i] = signer; } } - // @dev: Execute proposal calls and update state - // @param: calls - Array of cross-chain calls to execute - // @param: proposalHash - Hash of the proposal being executed + /// @dev Executes a batch of calls skipping local calls from other chains, updates the last executed batch hash + /// @dev In recovery mode, simply updates the last executed batch hash, unless the first call is `disableRecoveryMode` function _executeBatch(CrossChainCall[] memory calls, bytes32 batchHash) internal { if (!_isRecovery(calls[0])) { - // Execute each call in the batch uint256 len = calls.length; - for (uint256 i = 0; i < len; ++i) { - CrossChainCall memory call = calls[i]; - uint256 chainId = call.chainId; - + for (uint256 i; i < len; ++i) { + uint256 chainId = calls[i].chainId; if (chainId == 0 || chainId == block.chainid) { - Address.functionCall(call.target, call.callData, "Call execution failed"); + calls[i].target.functionCall(calls[i].callData, "Call execution failed"); } } } - _executedBatchHashes.push(batchHash); - lastBatchHash = batchHash; - emit ExecuteBatch(batchHash); } - // @dev: Check whether the batch needs to be skipped due to recovery mode - function _isRecovery(CrossChainCall memory call0) internal view returns (bool) { - if (!recoveryModeEnabled) return false; + // ------------------ // + // SIGNERS MANAGEMENT // + // ------------------ // - return !( - (call0.chainId == 0) && (call0.target == address(this)) - && (bytes4(call0.callData) == ICrossChainMultisig.disableRecoveryMode.selector) - ); + /// @notice Returns whether `account` is an approved signer + function isSigner(address account) external view override returns (bool) { + return _signersSet.contains(account); } - // - // MULTISIG CONFIGURATION FUNCTIONS - // - - // @notice: Add a new signer to the multisig - // @param: newSigner - Address of the new signer - function addSigner(address newSigner) external onlySelf { - _addSigner(newSigner); + /// @notice Returns list of approved signers + function getSigners() external view override returns (address[] memory) { + return _signersSet.values(); } - function _addSigner(address newSigner) internal { - if (!_signers.add(newSigner)) revert SignerAlreadyExistsException(); - emit AddSigner(newSigner); + /// @notice Adds `signer` to the list of approved signers + /// @dev Can only be called by the contract itself + /// @dev Reverts if signer is zero address or is already approved + function addSigner(address signer) external override onlySelf { + _addSigner(signer); } - // @notice: Remove a signer from the multisig - // @param: signer - Address of the signer to remove - function removeSigner(address signer) external onlySelf { - if (!_signers.remove(signer)) revert SignerDoesNotExistException(); - if (_signers.length() < confirmationThreshold) revert InvalidConfirmationThresholdValueException(); + /// @notice Removes `signer` from the list of approved signers + /// @dev Can only be called by the contract itself + /// @dev Reverts if signer is not approved + /// @dev Reverts if removing the signer makes multisig have less than `confirmationThreshold` signers + function removeSigner(address signer) external override onlySelf { + if (!_signersSet.remove(signer)) revert SignerIsNotApprovedException(signer); + if (_signersSet.length() < confirmationThreshold) revert InvalidConfirmationThresholdException(); emit RemoveSigner(signer); } - // @notice: Set the confirmation threshold for the multisig - // @param: newConfirmationThreshold - New confirmation threshold - function setConfirmationThreshold(uint8 newConfirmationThreshold) external onlySelf { + /// @notice Sets the minimum number of signatures required to execute a batch to `newConfirmationThreshold` + /// @dev Can only be called by the contract itself + /// @dev Reverts if the new confirmation threshold is 0 or greater than the number of signers + function setConfirmationThreshold(uint8 newConfirmationThreshold) external override onlySelf { _setConfirmationThreshold(newConfirmationThreshold); } - function _setConfirmationThreshold(uint8 newConfirmationThreshold) internal { - if (newConfirmationThreshold == 0 || newConfirmationThreshold > _signers.length()) { - revert InvalidConfirmationThresholdValueException(); - } - confirmationThreshold = newConfirmationThreshold; // U:[SM-1] - emit SetConfirmationThreshold(newConfirmationThreshold); // U:[SM-1] + /// @dev `addSigner` implementation + function _addSigner(address signer) internal { + if (signer == address(0)) revert InvalidSignerAddressException(); + if (!_signersSet.add(signer)) revert SignerIsAlreadyApprovedException(signer); + emit AddSigner(signer); } - function disableRecoveryMode() external onlySelf { - if (recoveryModeEnabled) { - recoveryModeEnabled = false; - emit DisableRecoveryMode(); + /// @dev `setConfirmationThreshold` implementation + function _setConfirmationThreshold(uint8 newConfirmationThreshold) internal { + if (newConfirmationThreshold == 0 || newConfirmationThreshold > _signersSet.length()) { + revert InvalidConfirmationThresholdException(); } + if (newConfirmationThreshold == confirmationThreshold) return; + confirmationThreshold = newConfirmationThreshold; + emit SetConfirmationThreshold(newConfirmationThreshold); } - // - // HELPERS - // - function hashBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) - public - pure - returns (bytes32) + /// @dev Ensures that the list of signatures has no duplicates and returns the number of valid signatures + function _verifySignatures(bytes[] memory signatures, bytes32 digest) + internal + view + returns (uint256 validSignatures) { - bytes32[] memory callsHash = new bytes32[](calls.length); - uint256 len = calls.length; - for (uint256 i = 0; i < len; ++i) { - CrossChainCall memory call = calls[i]; - callsHash[i] = keccak256(abi.encode(CROSS_CHAIN_CALL_TYPEHASH, call.chainId, call.target, call.callData)); + uint256 len = signatures.length; + address[] memory signers = new address[](len); + for (uint256 i; i < len; ++i) { + address signer = ECDSA.recover(digest, signatures[i]); + if (_signersSet.contains(signer)) validSignatures++; + for (uint256 j; j < i; ++j) { + if (signers[j] == signer) revert DuplicateSignatureException(signer); + } + signers[i] = signer; } - - return keccak256(abi.encode(keccak256(bytes(name)), keccak256(abi.encodePacked(callsHash)), prevHash)); } - function computeSignBatchHash(string memory name, bytes32 batchHash, bytes32 prevHash) - public - pure - returns (bytes32) - { - return keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes(name)), batchHash, prevHash)); - } - - function computeRecoveryModeHash(bytes32 startingBatchHash) public pure returns (bytes32) { - return keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, startingBatchHash)); - } + // ------------- // + // RECOVERY MODE // + // ------------- // - // - // GETTERS - // - function getSigners() external view returns (address[] memory) { - return _signers.values(); - } + /// @notice Enables recovery mode, in which calls execution is skipped + /// @dev Can only be executed outside Ethereum Mainnet + /// @dev Reverts if starting batch of recovery mode is not the last executed batch + /// @dev Reverts if the number of signatures is insufficient + function enableRecoveryMode(SignedRecoveryModeMessage calldata message) + external + override + onlyNotOnMainnet + nonReentrant + { + if (isRecoveryModeEnabled) return; + if (message.startingBatchHash != lastBatchHash()) revert InvalidRecoveryModeMessageException(); - function getBatch(bytes32 batchHash) external view returns (SignedBatch memory result) { - return _signedBatches[batchHash]; - } + bytes32 digest = _hashTypedDataV4(computeRecoveryModeHash(message.startingBatchHash)); + uint256 validSignatures = _verifySignatures(message.signatures, digest); + if (validSignatures < confirmationThreshold) revert InsufficientNumberOfSignaturesException(); - function getCurrentBatchHashes() external view returns (bytes32[] memory) { - return _connectedBatchHashes[lastBatchHash].values(); + isRecoveryModeEnabled = true; + emit EnableRecoveryMode(message.startingBatchHash); } - function getExecutedBatchHashes() external view returns (bytes32[] memory) { - return _executedBatchHashes; + /// @notice Disables recovery mode + /// @dev Can only be executed by the contract itself + function disableRecoveryMode() external override onlySelf { + if (!isRecoveryModeEnabled) return; + isRecoveryModeEnabled = false; + emit DisableRecoveryMode(); } - function isSigner(address account) external view returns (bool) { - return _signers.contains(account); - } + /// @dev Whether the batch should be skipped due to recovery mode + function _isRecovery(CrossChainCall memory call0) internal view returns (bool) { + if (!isRecoveryModeEnabled) return false; - function domainSeparatorV4() external view returns (bytes32) { - return _domainSeparatorV4(); + return !( + (call0.chainId == 0) && (call0.target == address(this)) + && (bytes4(call0.callData) == ICrossChainMultisig.disableRecoveryMode.selector) + ); } } diff --git a/contracts/interfaces/ICrossChainMultisig.sol b/contracts/interfaces/ICrossChainMultisig.sol index 5005c4a..c1deedf 100644 --- a/contracts/interfaces/ICrossChainMultisig.sol +++ b/contracts/interfaces/ICrossChainMultisig.sol @@ -6,152 +6,87 @@ pragma solidity ^0.8.23; import {CrossChainCall, SignedBatch, SignedRecoveryModeMessage} from "./Types.sol"; import {IVersion} from "@gearbox-protocol/core-v3/contracts/interfaces/base/IVersion.sol"; +/// @title Cross-chain multisig interface interface ICrossChainMultisig is IVersion { - // - // Events - // - /// @notice Emitted when a new signer is added to the multisig - /// @param signer Address of the newly added signer - event AddSigner(address indexed signer); + // ------ // + // EVENTS // + // ------ // - /// @notice Emitted when a signer is removed from the multisig - /// @param signer Address of the removed signer + event AddSigner(address indexed signer); + event DisableRecoveryMode(); + event EnableRecoveryMode(bytes32 indexed startingBatchHash); + event ExecuteBatch(bytes32 indexed batchHash); event RemoveSigner(address indexed signer); - - /// @notice Emitted when the confirmation threshold is updated - /// @param newconfirmationThreshold New number of required signatures - event SetConfirmationThreshold(uint8 newconfirmationThreshold); - - /// @notice Emitted when a new batch is submitted - /// @param batchHash Hash of the submitted batch - event SubmitBatch(bytes32 indexed batchHash); - - /// @notice Emitted when a signer signs a batch - /// @param batchHash Hash of the signed batch - /// @param signer Address of the signer + event SetConfirmationThreshold(uint8 newConfirmationThreshold); event SignBatch(bytes32 indexed batchHash, address indexed signer); + event SubmitBatch(bytes32 indexed batchHash); - /// @notice Emitted when a batch is successfully executed - /// @param batchHash Hash of the executed batch - event ExecuteBatch(bytes32 indexed batchHash); - - /// @notice Emitted when recovery mode is enabled - /// @param startingBatchHash Hash of the starting batch - event EnableRecoveryMode(bytes32 indexed startingBatchHash); - - /// @notice Emitted when recovery mode is disabled - event DisableRecoveryMode(); - - // Errors - - /// @notice Thrown when an invalid confirmation threshold is set - error InvalidconfirmationThresholdException(); - - /// @notice Thrown when a signer attempts to sign a proposal multiple times - error AlreadySignedException(); - - /// @notice Thrown when the previous proposal hash doesn't match the expected value - error InvalidPrevHashException(); - - /// @notice Thrown when trying to interact with a non-existent batch - error BatchDoesNotExistException(); - - /// @notice Thrown when trying to add a signer that already exists - error SignerAlreadyExistsException(); - - /// @notice Thrown when trying to remove a non-existent signer - error SignerDoesNotExistException(); + // ------ // + // ERRORS // + // ------ // - /// @notice Thrown when trying to execute a proposal on the wrong chain + error BatchIsNotSubmittedException(bytes32 batchHash); + error CallerIsNotSelfException(address caller); error CantBeExecutedOnCurrentChainException(); - - /// @notice Thrown when a restricted function is called by non-multisig address - error OnlySelfException(); - - /// @notice Thrown when submitting a proposal with no calls - error NoCallsInProposalException(); - - /// @notice Thrown when trying to execute a batch with insufficient signatures - error NotEnoughSignaturesException(); - - /// @notice Thrown when self-calls are inconsistent with the target chain - error InconsistentSelfCallOnOtherChainException(); - - /// @notice Thrown when setting an invalid confirmation threshold value - error InvalidConfirmationThresholdValueException(); - - /// @notice Thrown when attempting to start recovery mode with an incorrect starting batch hash + error DuplicateSignatureException(address signer); + error InsufficientNumberOfSignaturesException(); + error InvalidBatchException(); + error InvalidConfirmationThresholdException(); + error InvalidPrevHashException(); error InvalidRecoveryModeMessageException(); + error InvalidSignerAddressException(); + error SignerIsAlreadyApprovedException(address signer); + error SignerIsNotApprovedException(address signer); - /// @notice Submits a new batch to the multisig - /// @param calls Array of cross-chain calls to be executed - /// @param prevHash Hash of the previous batch (for ordering) - function submitBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) external; - - /// @notice Allows a signer to sign a submitted batch - /// @param batchHash Hash of the batch to sign - /// @param signature Signature of the signer - function signBatch(bytes32 batchHash, bytes calldata signature) external; + // --------------- // + // EIP-712 GETTERS // + // --------------- // - /// @notice Executes a batch once it has enough signatures - /// @param batch The signed batch to execute - function executeBatch(SignedBatch calldata batch) external; - - /// @notice Enables recovery mode - /// @param message Recover mode message with starting batch hash and signatures - function enableRecoveryMode(SignedRecoveryModeMessage memory message) external; - - /// @notice Adds a new signer to the multisig - /// @param signer Address of the signer to add - function addSigner(address signer) external; - - /// @notice Removes a signer from the multisig - /// @param signer Address of the signer to remove - function removeSigner(address signer) external; - - /// @notice Sets a new confirmation threshold - /// @param newThreshold New threshold value - function setConfirmationThreshold(uint8 newThreshold) external; - - /// @notice Disables recovery mode - function disableRecoveryMode() external; - - /// @notice Hashes a batch according to EIP-712 - /// @param name Name of the batch - /// @param calls Array of cross-chain calls - /// @param prevHash Hash of the previous batch - /// @return bytes32 Hash of the batch - function hashBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) + function domainSeparatorV4() external view returns (bytes32); + function CROSS_CHAIN_CALL_TYPEHASH() external view returns (bytes32); + function BATCH_TYPEHASH() external view returns (bytes32); + function COMPACT_BATCH_TYPEHASH() external view returns (bytes32); + function RECOVERY_MODE_TYPEHASH() external view returns (bytes32); + function computeCrossChainCallHash(CrossChainCall calldata call) external view returns (bytes32); + function computeBatchHash(string memory name, CrossChainCall[] calldata calls, bytes32 prevHash) external view returns (bytes32); + function computeCompactBatchHash(string memory name, bytes32 batchHash, bytes32 prevHash) + external + view + returns (bytes32); + function computeRecoveryModeHash(bytes32 startingBatchHash) external view returns (bytes32); - // - // GETTERS - // - - /// @notice Returns the current confirmation threshold - function confirmationThreshold() external view returns (uint8); + // ---------- // + // GOVERNANCE // + // ---------- // - /// @notice Returns the hash of the last executed batch function lastBatchHash() external view returns (bytes32); - - /// @notice Returns the array of executed batch hashes function getExecutedBatchHashes() external view returns (bytes32[] memory); - - /// @notice Returns all currently pending batches function getCurrentBatchHashes() external view returns (bytes32[] memory); - - /// @notice Returns a single executed batch + function getConnectedBatchHashes(bytes32 batchHash) external view returns (bytes32[] memory); function getBatch(bytes32 batchHash) external view returns (SignedBatch memory); + function submitBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) external; + function signBatch(bytes32 batchHash, bytes calldata signature) external; + function executeBatch(SignedBatch calldata batch) external; - /// @notice Returns the list of current signers - function getSigners() external view returns (address[] memory); + // ------------------ // + // SIGNERS MANAGEMENT // + // ------------------ // - /// @notice Checks if an address is a signer - /// @param account Address to check function isSigner(address account) external view returns (bool); + function getSigners() external view returns (address[] memory); + function confirmationThreshold() external view returns (uint8); + function addSigner(address signer) external; + function removeSigner(address signer) external; + function setConfirmationThreshold(uint8 newThreshold) external; - /// @notice Returns the domain separator used for EIP-712 signing - function domainSeparatorV4() external view returns (bytes32); + // ------------- // + // RECOVERY MODE // + // ------------- // + + function isRecoveryModeEnabled() external view returns (bool); + function enableRecoveryMode(SignedRecoveryModeMessage calldata message) external; + function disableRecoveryMode() external; } diff --git a/contracts/test/global/CrossChainMultisig.unit.t.sol b/contracts/test/global/CrossChainMultisig.unit.t.sol index 82ed5f8..ecaa530 100644 --- a/contracts/test/global/CrossChainMultisig.unit.t.sol +++ b/contracts/test/global/CrossChainMultisig.unit.t.sol @@ -19,7 +19,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { uint8 constant THRESHOLD = 2; address owner; - bytes32 BATCH_TYPEHASH = keccak256("Batch(string name,bytes32 batchHash,bytes32 prevHash)"); + bytes32 COMPACT_BATCH_TYPEHASH = keccak256("CompactBatch(string name,bytes32 batchHash,bytes32 prevHash)"); bytes32 RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(bytes32 startingBatchHash)"); function setUp() public { @@ -45,7 +45,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { view returns (bytes memory) { - bytes32 batchHash = multisig.hashBatch("test", calls, prevHash); + bytes32 batchHash = multisig.computeBatchHash("test", calls, prevHash); bytes32 structHash = _getDigest(batchHash); (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, structHash); return abi.encodePacked(r, s, v); @@ -106,15 +106,18 @@ contract CrossChainMultisigTest is Test, SignatureHelper { ); // Test onlySelf modifier - vm.expectRevert(ICrossChainMultisig.OnlySelfException.selector); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.CallerIsNotSelfException.selector, address(this))); multisig.addSigner(address(0x123)); - vm.expectRevert(ICrossChainMultisig.OnlySelfException.selector); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.CallerIsNotSelfException.selector, address(this))); multisig.removeSigner(signers[0]); - vm.expectRevert(ICrossChainMultisig.OnlySelfException.selector); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.CallerIsNotSelfException.selector, address(this))); multisig.setConfirmationThreshold(3); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.CallerIsNotSelfException.selector, address(this))); + multisig.disableRecoveryMode(); + // Test onlyOwner modifier vm.prank(makeAddr("notOwner")); vm.expectRevert("Ownable: caller is not the owner"); @@ -123,17 +126,29 @@ contract CrossChainMultisigTest is Test, SignatureHelper { /// @dev U:[SM-3]: Submit batch works correctly function test_CCG_03_SubmitBatch() public { - vm.startPrank(owner); vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); + bytes32 expectedBatchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + + vm.expectEmit(true, true, true, true); + emit ICrossChainMultisig.SubmitBatch(expectedBatchHash); + + vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); - SignedBatch memory batch = multisig.getBatch(batchHash); + SignedBatch memory batch = multisig.getBatch(expectedBatchHash); + assertEq(batch.calls.length, 1); + assertEq(batch.prevHash, bytes32(0)); + assertEq(batch.signatures.length, 0); + // submit the same batch again doesn't change calls + vm.prank(owner); + multisig.submitBatch("test", calls, bytes32(0)); + + batch = multisig.getBatch(expectedBatchHash); assertEq(batch.calls.length, 1); assertEq(batch.prevHash, bytes32(0)); assertEq(batch.signatures.length, 0); @@ -156,7 +171,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { CrossChainCall[] memory calls = new CrossChainCall[](0); - vm.expectRevert(ICrossChainMultisig.NoCallsInProposalException.selector); + vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); multisig.submitBatch("test", calls, bytes32(0)); } @@ -170,12 +185,13 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); console.log(signers[0]); console.logBytes32(batchHash); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Generate EIP-712 signature bytes memory signature = _signBatchHash(signer0PrivateKey, structHash); @@ -202,7 +218,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); // Try to sign with invalid signature bytes memory invalidSig = hex"1234"; @@ -220,7 +236,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { bytes32 nonExistentHash = keccak256("non-existent"); bytes memory signature = _signBatchHash(signer0PrivateKey, nonExistentHash); - vm.expectRevert(ICrossChainMultisig.InvalidPrevHashException.selector); + vm.expectRevert( + abi.encodeWithSelector(ICrossChainMultisig.BatchIsNotSubmittedException.selector, nonExistentHash) + ); multisig.signBatch(nonExistentHash, signature); } @@ -234,16 +252,17 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Sign first time bytes memory signature = _signBatchHash(signer0PrivateKey, structHash); multisig.signBatch(batchHash, signature); // Try to sign again with same signer - vm.expectRevert(ICrossChainMultisig.AlreadySignedException.selector); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.DuplicateSignatureException.selector, signers[0])); multisig.signBatch(batchHash, signature); } @@ -261,9 +280,10 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Sign with first signer bytes memory sig0 = _signBatchHash(signer0PrivateKey, structHash); @@ -298,7 +318,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { function test_CCG_13_VerifyBatchEmptyCalls() public { CrossChainCall[] memory calls = new CrossChainCall[](0); - vm.expectRevert(ICrossChainMultisig.NoCallsInProposalException.selector); + vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); multisig.exposed_verifyBatch(calls, bytes32(0)); } @@ -312,7 +332,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { callData: hex"1234" }); - vm.expectRevert(ICrossChainMultisig.InconsistentSelfCallOnOtherChainException.selector); + vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); multisig.exposed_verifyBatch(calls, bytes32(0)); } @@ -350,9 +370,10 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Create array with 2 valid signatures bytes[] memory signatures = new bytes[](2); @@ -371,9 +392,10 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Create array with 1 valid and 1 invalid signature bytes[] memory signatures = new bytes[](2); @@ -385,7 +407,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { uint256 validCount = multisig.exposed_verifySignatures(signatures, _getDigest(structHash)); assertEq(validCount, 1); } - /// @dev U:[SM-19]: _verifySignatures reverts with AlreadySignedException on duplicate signatures from same signer + /// @dev U:[SM-19]: _verifySignatures reverts with DuplicateSignatureException on duplicate signatures from same signer function test_CCG_19_VerifySignaturesDuplicateSigner() public { vm.chainId(1); // Set to mainnet @@ -394,9 +416,10 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Create array with 2 signatures from same signer bytes[] memory signatures = new bytes[](2); @@ -405,7 +428,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { bytes32 digest = _getDigest(structHash); - vm.expectRevert(ICrossChainMultisig.AlreadySignedException.selector); + vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.DuplicateSignatureException.selector, signers[0])); multisig.exposed_verifySignatures(signatures, digest); } @@ -417,9 +440,10 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(owner); multisig.submitBatch("test", calls, bytes32(0)); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); // Create random non-signer private key uint256 nonSignerKey = uint256(keccak256("non-signer")); @@ -456,8 +480,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); @@ -475,7 +500,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); - assertTrue(multisig.recoveryModeEnabled()); + assertTrue(multisig.isRecoveryModeEnabled()); } /// @dev U:[SM-23]: Recovery mode skips batch execution except for lastBatchHash update @@ -491,8 +516,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); @@ -515,8 +541,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch2 = SignedBatch({name: "test2", calls: calls2, prevHash: batchHash, signatures: new bytes[](2)}); - bytes32 batchHash2 = multisig.hashBatch("test2", calls2, batchHash); - bytes32 structHash2 = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); + bytes32 batchHash2 = multisig.computeBatchHash("test2", calls2, batchHash); + bytes32 structHash2 = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); batch2.signatures[0] = _signBatchHash(signer0PrivateKey, structHash2); batch2.signatures[1] = _signBatchHash(signer1PrivateKey, structHash2); @@ -525,7 +552,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { // Verify lastBatchHash was updated but call wasn't executed assertEq(multisig.lastBatchHash(), batchHash2); - assertTrue(multisig.recoveryModeEnabled()); + assertTrue(multisig.isRecoveryModeEnabled()); assertEq(GeneralMock(calledContract).data().length, 0); } @@ -542,8 +569,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); @@ -572,8 +600,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch2 = SignedBatch({name: "test2", calls: calls2, prevHash: batchHash, signatures: new bytes[](2)}); - bytes32 batchHash2 = multisig.hashBatch("test2", calls2, batchHash); - bytes32 structHash2 = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); + bytes32 batchHash2 = multisig.computeBatchHash("test2", calls2, batchHash); + bytes32 structHash2 = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); batch2.signatures[0] = _signBatchHash(signer0PrivateKey, structHash2); batch2.signatures[1] = _signBatchHash(signer1PrivateKey, structHash2); @@ -584,7 +613,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.executeBatch(batch2); // Verify recovery mode was disabled and both calls were executed - assertFalse(multisig.recoveryModeEnabled()); + assertFalse(multisig.isRecoveryModeEnabled()); assertEq(multisig.lastBatchHash(), batchHash2); assertEq(GeneralMock(calledContract).data(), hex"1234"); } @@ -618,8 +647,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); - bytes32 batchHash = multisig.hashBatch("test", calls, bytes32(0)); - bytes32 structHash = keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 structHash = + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); @@ -632,7 +662,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { bytes[] memory signatures = new bytes[](1); signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); - vm.expectRevert(ICrossChainMultisig.NotEnoughSignaturesException.selector); + vm.expectRevert(ICrossChainMultisig.InsufficientNumberOfSignaturesException.selector); multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); } @@ -640,7 +670,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.prank(address(multisig)); multisig.removeSigner(signers[0]); - vm.expectRevert(ICrossChainMultisig.InvalidConfirmationThresholdValueException.selector); + vm.expectRevert(ICrossChainMultisig.InvalidConfirmationThresholdException.selector); vm.prank(address(multisig)); multisig.removeSigner(signers[1]); } diff --git a/contracts/test/global/CrossChainMultisigHarness.sol b/contracts/test/global/CrossChainMultisigHarness.sol index c2f2990..0be4cda 100644 --- a/contracts/test/global/CrossChainMultisigHarness.sol +++ b/contracts/test/global/CrossChainMultisigHarness.sol @@ -32,6 +32,6 @@ contract CrossChainMultisigHarness is CrossChainMultisig { // Add setter for lastBatchHash function setLastBatchHash(bytes32 newHash) external { - lastBatchHash = newHash; + _executedBatchHashes.push(newHash); } } diff --git a/contracts/test/helpers/CCGHelper.sol b/contracts/test/helpers/CCGHelper.sol index bed85f8..3f1fbcd 100644 --- a/contracts/test/helpers/CCGHelper.sol +++ b/contracts/test/helpers/CCGHelper.sol @@ -19,7 +19,7 @@ contract CCGHelper is SignatureHelper { using LibString for uint256; // Core contracts - bytes32 constant BATCH_TYPEHASH = keccak256("Batch(string name,bytes32 batchHash,bytes32 prevHash)"); + bytes32 constant COMPACT_BATCH_TYPEHASH = keccak256("CompactBatch(string name,bytes32 batchHash,bytes32 prevHash)"); CrossChainMultisig internal multisig; @@ -104,10 +104,9 @@ contract CCGHelper is SignatureHelper { SignedBatch memory currentBatch = multisig.getBatch(currentBatchHashes[0]); - bytes32 batchHash = multisig.hashBatch(currentBatch.name, currentBatch.calls, currentBatch.prevHash); + bytes32 batchHash = multisig.computeBatchHash(currentBatch.name, currentBatch.calls, currentBatch.prevHash); - bytes32 structHash = - keccak256(abi.encode(BATCH_TYPEHASH, keccak256(bytes(currentBatch.name)), batchHash, currentBatch.prevHash)); + bytes32 structHash = multisig.computeCompactBatchHash(currentBatch.name, batchHash, currentBatch.prevHash); if (!_isTestMode()) { console.log("tt"); From 7dc41e16c392a561612735a6fe8e7a72789cd756 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Thu, 27 Feb 2025 17:28:06 +0200 Subject: [PATCH 3/5] feat: solve some `BytecodeRepository` issues - make system more robust against instance operator strategically placing `uploadBytecode` and `submitAuditReport` within governance transactions - cleaner separation between system and public domains (they are either system or public and it's one-way now, only public contracts have owner) - more comprehensible protection against domain squatting by compromised auditor - better contract type and version validation and control --- contracts/global/BytecodeRepository.sol | 216 ++++++++++-------- contracts/interfaces/IBytecodeRepository.sol | 41 ++-- contracts/libraries/Domain.sol | 51 ++++- .../test/global/BytecodeRepository.unit.t.sol | 34 --- contracts/test/libraries/Domain.unit.t.sol | 93 ++++++-- contracts/traits/DeployerTrait.sol | 4 +- 6 files changed, 264 insertions(+), 175 deletions(-) diff --git a/contracts/global/BytecodeRepository.sol b/contracts/global/BytecodeRepository.sol index 962a838..876ebd6 100644 --- a/contracts/global/BytecodeRepository.sol +++ b/contracts/global/BytecodeRepository.sol @@ -25,6 +25,7 @@ import {ImmutableOwnableTrait} from "../traits/ImmutableOwnableTrait.sol"; contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecodeRepository, EIP712Mainnet { using EnumerableSet for EnumerableSet.AddressSet; using EnumerableSet for EnumerableSet.Bytes32Set; + using EnumerableSet for EnumerableSet.UintSet; using LibString for bytes32; using LibString for string; using LibString for uint256; @@ -32,9 +33,11 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod /// @dev Internal struct with version info for a given contract type struct VersionInfo { + address owner; uint256 latest; - mapping(uint256 => uint256) latestByMajor; - mapping(uint256 => uint256) latestByMinor; + mapping(uint256 majorVersion => uint256) latestByMajor; + mapping(uint256 minorVersion => uint256) latestByMinor; + EnumerableSet.UintSet versionsSet; } /// @notice Contract version @@ -63,11 +66,8 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod /// @dev Mapping from `cType` to `ver` to allowed bytecode hash mapping(bytes32 cType => mapping(uint256 ver => bytes32 bytecodeHash)) internal _allowedBytecodeHashes; - /// @dev Mapping from `cType` to its owner - mapping(bytes32 cType => address owner) internal _contractTypeOwners; - - /// @dev Mapping from `bytecodeHash` to whether it is allowed as system contract - mapping(bytes32 bytecodeHash => bool) internal _isAllowedSystemContract; + /// @dev Set of system domains + EnumerableSet.Bytes32Set internal _systemDomainsSet; /// @dev Set of public domains EnumerableSet.Bytes32Set internal _publicDomainsSet; @@ -161,9 +161,10 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod /// @notice Deploys a contract of a given type and version with given constructor parameters and salt. /// Tries to transfer ownership over the deployed contract to the caller. - /// Bytecode must be allowed, which means it is either a system contract or a contract from public - /// domain with at least one signed report from approved auditor and not forbidden init code. + /// Bytecode must be allowed either as system or public contract, which, in turn, requires it + /// to be uploaded and have at least one signed report from approved auditor. /// @dev Deployer's address is mixed with salt to prevent front-running using collisions + /// @dev Reverts if contract's init code is forbidden /// @dev Reverts if contract was previously deployed at the same address /// @dev Reverts if deployed contract's type or version does not match passed parameters function deploy(bytes32 cType, uint256 ver, bytes memory constructorParams, bytes32 salt) @@ -220,6 +221,7 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod /// @notice Uploads new contract bytecode to the repository. /// Simply uploading the bytecode is not enough to deploy a contract with it, see `deploy` for details. + /// @dev Reverts if bytecode's contract type is invalid or version is less than `100` or greater than `999` /// @dev Reverts if bytecode for given contract type and version is already allowed /// @dev Reverts if author is zero address or if their signature is invalid /// @dev On mainnet, only author of the bytecode can upload it @@ -227,6 +229,8 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod bytes32 bytecodeHash = computeBytecodeHash(bytecode); if (isBytecodeUploaded(bytecodeHash)) return; + _validateContractType(bytecode.contractType); + _validateVersion(bytecode.contractType, bytecode.version); if (_allowedBytecodeHashes[bytecode.contractType][bytecode.version] != 0) { revert BytecodeIsAlreadyAllowedException(bytecode.contractType, bytecode.version); } @@ -296,23 +300,12 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod } reports.push(auditReport); emit AuditBytecode(bytecodeHash, auditor, auditReport.reportUrl); - - _doStuff(bytecodeHash); } // ----------------- // // ALLOWING BYTECODE // // ----------------- // - // TODO: rework this section - // some issues: - // - nothing prevents owner from calling `allowSystemContract` twice for the same contract type and version, - // overwriting the contract type owner - // - the order in which `addPublicDomain` and `submitAuditReport` are called is important as submitting the - // report before adding the public domain leaves us with unallowed contract unless we directly allow it - // as system contract or find another auditor signature (with fake report URL or idk) - // - the behavior is a bit unpredictable if owner allows system contract which has a type from public domain - /// @notice Returns the allowed bytecode hash for `cType` and `ver` function getAllowedBytecodeHash(bytes32 cType, uint256 ver) external view override returns (bytes32) { return _allowedBytecodeHashes[cType][ver]; @@ -320,91 +313,105 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod /// @notice Returns the owner of `cType` function getContractTypeOwner(bytes32 cType) external view override returns (address) { - return _contractTypeOwners[cType]; - } - - /// @notice Returns whether contract with `bytecodeHash` is an allowed system contract - function isAllowedSystemContract(bytes32 bytecodeHash) external view override returns (bool) { - return _isAllowedSystemContract[bytecodeHash]; + return _versionInfo[cType].owner; } - /// @notice Allows owner to mark contracts as system contracts - /// @param bytecodeHash Hash of the bytecode metadata to allow + /// @notice Marks bytecode with `bytecodeHash` as allowed system contract. + /// Adds bytecode's domain to the list of system domains. + /// @dev Can only be called by the owner + /// @dev Reverts if bytecode is not uploaded or not audited + /// @dev Reverts if bytecode's contract type is in the list of public domains + /// @dev Reverts if bytecode with this contract type and version is already allowed function allowSystemContract(bytes32 bytecodeHash) external override onlyOwner { - if (!isBytecodeUploaded(bytecodeHash) || !isBytecodeAudited(bytecodeHash)) return; - if (_isAllowedSystemContract[bytecodeHash]) return; + if (!isBytecodeUploaded(bytecodeHash)) revert BytecodeIsNotUploadedException(bytecodeHash); + if (!isBytecodeAudited(bytecodeHash)) revert BytecodeIsNotAuditedException(bytecodeHash); - _isAllowedSystemContract[bytecodeHash] = true; BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; bytes32 cType = bytecode.contractType; - address author = bytecode.author; + _addSystemDomain(cType.extractDomain()); - // QUESTION: should we check if `cType` already has an owner? - // because it's in public domain or was previously allowed as system contract - _contractTypeOwners[cType] = author; - emit SetContractTypeOwner(cType, author); - - _allowBytecode(bytecodeHash, cType, bytecode.version); + _allowContract(bytecodeHash, cType, bytecode.version); } - function revokeApproval(bytes32 cType, uint256 ver, bytes32 bytecodeHash) external override onlyOwner { - if (_allowedBytecodeHashes[cType][ver] == bytecodeHash) { - _allowedBytecodeHashes[cType][ver] = bytes32(0); - emit ForbidBytecode(bytecodeHash, cType, ver); - } - } - - /// @notice Removes contract type owner - /// @param cType Contract type to remove owner from - /// @dev Used to remove malicious auditors and cybersquatters - function removeContractTypeOwner(bytes32 cType) external override onlyOwner { - if (_contractTypeOwners[cType] == address(0)) return; - _contractTypeOwners[cType] = address(0); - emit RemoveContractTypeOwner(cType); - } + /// @notice Marks bytecode with `bytecodeHash` as allowed public contract. + /// Sets bytecode's author as contract type owner. + /// @dev Reverts if bytecode is not uploaded or not audited + /// @dev Reverts if bytecode's contract type is not in the list of public domains + /// @dev Reverts if bytecode's author is not contract type owner + /// @dev Reverts if bytecode with this contract type and version is already allowed + function allowPublicContract(bytes32 bytecodeHash) external override { + if (!isBytecodeUploaded(bytecodeHash)) revert BytecodeIsNotUploadedException(bytecodeHash); + if (!isBytecodeAudited(bytecodeHash)) revert BytecodeIsNotAuditedException(bytecodeHash); - function _doStuff(bytes32 bytecodeHash) internal { BytecodePointer storage bytecode = _bytecodeByHash[bytecodeHash]; bytes32 cType = bytecode.contractType; - - bool isSystemContract = _isAllowedSystemContract[bytecodeHash]; - bool isContractTypeInPublicDomain = isInPublicDomain(cType); + if (!isPublicDomain(cType.extractDomain())) revert ContractTypeIsNotInPublicDomainException(cType); address author = bytecode.author; - address currentOwner = _contractTypeOwners[cType]; - if (isSystemContract && currentOwner == address(0)) { - _contractTypeOwners[cType] = author; + address contractTypeOwner = _versionInfo[cType].owner; + if (contractTypeOwner == address(0)) { + _versionInfo[cType].owner = author; emit SetContractTypeOwner(cType, author); - } else if (isContractTypeInPublicDomain && currentOwner != author) { + } else if (contractTypeOwner != author) { revert AuthorIsNotContractTypeOwnerException(cType, author); } - // FIXME: for contracts from public domain, submitting a signature before adding the public domain leaves us - // with no other way to later allow it except marking it as system contract or finding another auditor signature - if (isSystemContract || isContractTypeInPublicDomain) _allowBytecode(bytecodeHash, cType, bytecode.version); + _allowContract(bytecodeHash, cType, bytecode.version); } - function _allowBytecode(bytes32 bytecodeHash, bytes32 cType, uint256 ver) internal { - // QUESTION: what if bytecodeHash is non-zero but different from already set? - if (_allowedBytecodeHashes[cType][ver] != 0) return; + /// @notice Forbids all previously allowed public contracts of a given type, removes type owner and version info. + /// Exists primarily to cleanup the repository after public domain squatting by a compromised auditor. + /// @dev Can only be called by the owner + function removePublicContractType(bytes32 cType) external override onlyOwner { + if (!isPublicDomain(cType.extractDomain())) return; + + VersionInfo storage info = _versionInfo[cType]; + if (info.owner != address(0)) { + info.owner = address(0); + emit RemoveContractTypeOwner(cType); + } + info.latest = 0; + uint256[] memory versions = info.versionsSet.values(); + uint256 numVersions = versions.length; + for (uint256 i; i < numVersions; ++i) { + uint256 ver = versions[i]; + info.versionsSet.remove(ver); + info.latestByMajor[_getMajorVersion(ver)] = 0; + info.latestByMinor[_getMinorVersion(ver)] = 0; + + bytes32 bytecodeHash = _allowedBytecodeHashes[cType][ver]; + _allowedBytecodeHashes[cType][ver] = bytes32(0); + emit ForbidContract(bytecodeHash, cType, ver); + } + } + /// @dev Allows bytecode with `bytecodeHash` for `cType` and `ver`, updates version info for `cType` + /// @dev Reverts if bytecode is already allowed + function _allowContract(bytes32 bytecodeHash, bytes32 cType, uint256 ver) internal { + if (_allowedBytecodeHashes[cType][ver] == bytecodeHash) return; + if (_allowedBytecodeHashes[cType][ver] != 0) revert BytecodeIsAlreadyAllowedException(cType, ver); _allowedBytecodeHashes[cType][ver] = bytecodeHash; - emit AllowBytecode(bytecodeHash, cType, ver); + emit AllowContract(bytecodeHash, cType, ver); _updateVersionInfo(cType, ver); } - // ------------------------- // - // PUBLIC DOMAINS MANAGEMENT // - // ------------------------- // + // ------------------ // + // DOMAINS MANAGEMENT // + // ------------------ // + + /// @notice Whether `domain` is in the list of system domains + function isSystemDomain(bytes32 domain) public view override returns (bool) { + return _systemDomainsSet.contains(domain); + } - /// @notice Whether `cType` belongs to a public domain - function isInPublicDomain(bytes32 cType) public view override returns (bool) { - return _publicDomainsSet.contains(cType.extractDomain()); + /// @notice Returns list of all system domains + function getSystemDomains() external view override returns (bytes32[] memory) { + return _systemDomainsSet.values(); } /// @notice Whether `domain` is in the list of public domains - function isPublicDomain(bytes32 domain) external view override returns (bool) { + function isPublicDomain(bytes32 domain) public view override returns (bool) { return _publicDomainsSet.contains(domain); } @@ -413,17 +420,26 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod return _publicDomainsSet.values(); } - /// @notice Adds `domain` to the list of public domains (does nothing if `domain` contains "::") + /// @notice Adds `domain` to the list of public domains /// @dev Can only be called by the owner + /// @dev Reverts if `domain` is invalid or is already in the list of system domains function addPublicDomain(bytes32 domain) external override onlyOwner { - if (domain == bytes32(0) || LibString.fromSmallString(domain).contains("::")) return; + _validateDomain(domain); + _addPublicDomain(domain); + } + + /// @dev Adds `domain` to the list of public domains + /// @dev Reverts if `domain` is already in the list of system domains + function _addPublicDomain(bytes32 domain) internal { + if (isSystemDomain(domain)) revert DomainIsAlreadyMarketAsSystemException(domain); if (_publicDomainsSet.add(domain)) emit AddPublicDomain(domain); } - /// @notice Removes `domain` from the list of public domains - /// @dev Can only be called by the owner - function removePublicDomain(bytes32 domain) external override onlyOwner { - if (_publicDomainsSet.remove(domain)) emit RemovePublicDomain(domain); + /// @dev Adds `domain` to the list of system domains + /// @dev Reverts if `domain` is already in the list of public domains + function _addSystemDomain(bytes32 domain) internal { + if (isPublicDomain(domain)) revert DomainIsAlreadyMarketAsPublicException(domain); + if (_systemDomainsSet.add(domain)) emit AddSystemDomain(domain); } // ------------------- // @@ -462,16 +478,16 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod emit RemoveAuditor(auditor); } - // ------------------- // - // FORBIDDING INITCODE // - // ------------------- // + // -------------------- // + // FORBIDDING INIT CODE // + // -------------------- // /// @notice Whether init code with `initCodeHash` is forbidden function isInitCodeForbidden(bytes32 initCodeHash) external view override returns (bool) { return _isInitCodeForbidden[initCodeHash]; } - /// @notice Marks init code with `initCodeHash` as forbidden + /// @notice Permanently marks init code with `initCodeHash` as forbidden /// @dev Can only be called by the owner function forbidInitCode(bytes32 initCodeHash) external override onlyOwner { if (_isInitCodeForbidden[initCodeHash]) return; @@ -506,6 +522,11 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod // VERSION CONTROL // // --------------- // + /// @notice Returns all versions for `cType` + function getVersions(bytes32 cType) external view override returns (uint256[] memory) { + return _versionInfo[cType].versionsSet.values(); + } + /// @notice Returns the latest known bytecode version for given `cType` /// @dev Reverts if `cType` has no bytecode entries function getLatestVersion(bytes32 cType) external view override returns (uint256 ver) { @@ -513,8 +534,8 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod if (ver == 0) revert VersionNotFoundException(cType); } - /// @notice Returns the latest known minor version for given `cType` and `majorVersion` - /// @dev Reverts if `majorVersion` is less than `100` + /// @notice Returns the latest known version for given `cType` with matching `majorVersion` + /// @dev Reverts if `majorVersion` is less than `100` or greater than `999` /// @dev Reverts if `cType` has no bytecode entries with matching `majorVersion` function getLatestMinorVersion(bytes32 cType, uint256 majorVersion) external view override returns (uint256 ver) { _validateVersion(cType, majorVersion); @@ -522,8 +543,8 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod if (ver == 0) revert VersionNotFoundException(cType); } - /// @notice Returns the latest known patch version for given `cType` and `minorVersion` - /// @dev Reverts if `minorVersion` is less than `100` + /// @notice Returns the latest known version for given `cType` with matching `minorVersion` + /// @dev Reverts if `minorVersion` is less than `100` or greater than `999` /// @dev Reverts if `cType` has no bytecode entries with matching `minorVersion` function getLatestPatchVersion(bytes32 cType, uint256 minorVersion) external view override returns (uint256 ver) { _validateVersion(cType, minorVersion); @@ -539,6 +560,7 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod if (ver > info.latestByMajor[majorVersion]) info.latestByMajor[majorVersion] = ver; uint256 minorVersion = _getMinorVersion(ver); if (ver > info.latestByMinor[minorVersion]) info.latestByMinor[minorVersion] = ver; + info.versionsSet.add(ver); } /// @dev Returns the major version of a given version @@ -551,8 +573,18 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod return ver - ver % 10; } - /// @dev Reverts if `ver` is less than `100` - function _validateVersion(bytes32 key, uint256 ver) internal pure { - if (ver < 100) revert InvalidVersionException(key, ver); + /// @dev Reverts if `cType` is invalid + function _validateContractType(bytes32 cType) internal pure { + if (!cType.isValidContractType()) revert InvalidContractTypeException(cType); + } + + /// @dev Reverts if `domain` is invalid + function _validateDomain(bytes32 domain) internal pure { + if (!domain.isValidDomain()) revert InvalidDomainException(domain); + } + + /// @dev Reverts if `ver` is less than `100` or greater than `999` + function _validateVersion(bytes32 cType, uint256 ver) internal pure { + if (ver < 100 || ver > 999) revert InvalidVersionException(cType, ver); } } diff --git a/contracts/interfaces/IBytecodeRepository.sol b/contracts/interfaces/IBytecodeRepository.sol index 4e0096d..8081b27 100644 --- a/contracts/interfaces/IBytecodeRepository.sol +++ b/contracts/interfaces/IBytecodeRepository.sol @@ -15,13 +15,17 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { event AddAuditor(address indexed auditor, string name); event AddPublicDomain(bytes32 indexed domain); + event AddSystemDomain(bytes32 indexed domain); + event AllowContract(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); event AuditBytecode(bytes32 indexed bytecodeHash, address indexed auditor, string reportUrl); event DeployContract( bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address contractAddress ); + event ForbidContract(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); event ForbidInitCode(bytes32 indexed initCodeHash); event RemoveAuditor(address indexed auditor); - event RemovePublicDomain(bytes32 indexed domain); + event RemoveContractTypeOwner(bytes32 indexed cType); + event SetContractTypeOwner(bytes32 indexed cType, address indexed owner); event SetTokenSpecificPostfix(address indexed token, bytes32 indexed postfix); event UploadBytecode( bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address author, string source @@ -36,13 +40,19 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { error BytecodeIsAlreadyAllowedException(bytes32 cType, uint256 ver); error BytecodeIsAlreadySignedByAuditorException(bytes32 bytecodeHash, address auditor); error BytecodeIsNotAllowedException(bytes32 cType, uint256 ver); + error BytecodeIsNotAuditedException(bytes32 bytecodeHash); error BytecodeIsNotUploadedException(bytes32 bytecodeHash); error CallerIsNotBytecodeAuthorException(address caller); error ContractIsAlreadyDeployedException(address deployedContract); + error ContractTypeIsNotInPublicDomainException(bytes32 cType); + error DomainIsAlreadyMarketAsPublicException(bytes32 domain); + error DomainIsAlreadyMarketAsSystemException(bytes32 domain); error InitCodeIsForbiddenException(bytes32 initCodeHash); error InvalidAuditorSignatureException(address auditor); error InvalidAuthorSignatureException(address author); error InvalidBytecodeException(bytes32 bytecodeHash); + error InvalidContractTypeException(bytes32 cType); + error InvalidDomainException(bytes32 domain); error InvalidVersionException(bytes32 cType, uint256 ver); error VersionNotFoundException(bytes32 cType); @@ -98,29 +108,21 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { // ALLOWING BYTECODE // // ----------------- // - // TODO: rework this section - - event AllowBytecode(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); - event ForbidBytecode(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); - event SetContractTypeOwner(bytes32 indexed cType, address indexed owner); - event RemoveContractTypeOwner(bytes32 indexed cType); - function getAllowedBytecodeHash(bytes32 cType, uint256 ver) external view returns (bytes32); - function isAllowedSystemContract(bytes32 bytecodeHash) external view returns (bool); function getContractTypeOwner(bytes32 cType) external view returns (address); function allowSystemContract(bytes32 bytecodeHash) external; - function revokeApproval(bytes32 cType, uint256 ver, bytes32 bytecodeHash) external; - function removeContractTypeOwner(bytes32 cType) external; + function allowPublicContract(bytes32 bytecodeHash) external; + function removePublicContractType(bytes32 cType) external; - // ------------------------- // - // PUBLIC DOMAINS MANAGEMENT // - // ------------------------- // + // ------------------ // + // DOMAINS MANAGEMENT // + // ------------------ // - function isInPublicDomain(bytes32 cType) external view returns (bool); + function isSystemDomain(bytes32 domain) external view returns (bool); + function getSystemDomains() external view returns (bytes32[] memory); function isPublicDomain(bytes32 domain) external view returns (bool); function getPublicDomains() external view returns (bytes32[] memory); function addPublicDomain(bytes32 domain) external; - function removePublicDomain(bytes32 domain) external; // ------------------- // // AUDITORS MANAGEMENT // @@ -132,9 +134,9 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { function addAuditor(address auditor, string calldata name) external; function removeAuditor(address auditor) external; - // ------------------- // - // FORBIDDING INITCODE // - // ------------------- // + // -------------------- // + // FORBIDDING INIT CODE // + // -------------------- // function isInitCodeForbidden(bytes32 initCodeHash) external view returns (bool); function forbidInitCode(bytes32 initCodeHash) external; @@ -150,6 +152,7 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { // VERSION CONTROL // // --------------- // + function getVersions(bytes32 cType) external view returns (uint256[] memory); function getLatestVersion(bytes32 cType) external view returns (uint256); function getLatestMinorVersion(bytes32 cType, uint256 majorVersion) external view returns (uint256); function getLatestPatchVersion(bytes32 cType, uint256 minorVersion) external view returns (uint256); diff --git a/contracts/libraries/Domain.sol b/contracts/libraries/Domain.sol index d440081..0df8294 100644 --- a/contracts/libraries/Domain.sol +++ b/contracts/libraries/Domain.sol @@ -9,18 +9,53 @@ library Domain { using LibString for string; using LibString for bytes32; - function extractDomain(string memory str) internal pure returns (string memory) { + uint128 internal constant UNDERSCORE = 1 << 95; + + function getContractType(bytes32 domain, bytes32 postfix) internal pure returns (bytes32) { + if (postfix == 0) return domain; + return string.concat(domain.fromSmallString(), "::", postfix.fromSmallString()).toSmallString(); + } + + function extractDomain(bytes32 contractType) internal pure returns (bytes32) { + string memory str = contractType.fromSmallString(); uint256 separatorIndex = str.indexOf("::"); - // If no separator found, treat the whole name as domain - if (separatorIndex == LibString.NOT_FOUND) { - return str; - } + // If no separator found, treat the whole type as domain + if (separatorIndex == LibString.NOT_FOUND) return str.toSmallString(); - return str.slice(0, separatorIndex); + return str.slice(0, separatorIndex).toSmallString(); } - function extractDomain(bytes32 contractType) internal pure returns (bytes32) { - return extractDomain(contractType.fromSmallString()).toSmallString(); + function extractPostfix(bytes32 contractType) internal pure returns (bytes32) { + string memory str = contractType.fromSmallString(); + uint256 separatorIndex = str.indexOf("::"); + + // if no separator found, return empty postfix + if (separatorIndex == LibString.NOT_FOUND) return bytes32(0); + + return str.slice(separatorIndex + 2).toSmallString(); + } + + function isValidContractType(bytes32 contractType) internal pure returns (bool) { + bytes32 domain = extractDomain(contractType); + if (!isValidDomain(domain)) return false; + + bytes32 postfix = extractPostfix(contractType); + if (!isValidPostfix(postfix)) return false; + + // avoid the "DOMAIN::" case + return contractType == getContractType(domain, postfix); + } + + function isValidDomain(bytes32 domain) internal pure returns (bool) { + return domain != 0 && _isValidString(domain.fromSmallString()); + } + + function isValidPostfix(bytes32 postfix) internal pure returns (bool) { + return _isValidString(postfix.fromSmallString()); + } + + function _isValidString(string memory str) internal pure returns (bool) { + return str.is7BitASCII(LibString.ALPHANUMERIC_7_BIT_ASCII | UNDERSCORE); } } diff --git a/contracts/test/global/BytecodeRepository.unit.t.sol b/contracts/test/global/BytecodeRepository.unit.t.sol index c831903..0fdd2f6 100644 --- a/contracts/test/global/BytecodeRepository.unit.t.sol +++ b/contracts/test/global/BytecodeRepository.unit.t.sol @@ -246,38 +246,4 @@ contract BytecodeRepositoryTest is Test, SignatureHelper { ); repository.deploy(_TEST_CONTRACT, _TEST_VERSION, "", _TEST_SALT); } - - function test_BCR_08_deploy_reverts_if_not_audited() public { - // Upload but don't audit - bytes memory bytecode = _getMockBytecode(_TEST_CONTRACT, _TEST_VERSION); - - Bytecode memory bc = Bytecode({ - contractType: _TEST_CONTRACT, - version: _TEST_VERSION, - initCode: bytecode, - author: author, - source: _TEST_SOURCE, - authorSignature: bytes("") - }); - - bytes32 bytecodeHash = repository.computeBytecodeHash(bc); - - (uint8 v, bytes32 r, bytes32 s) = - vm.sign(authorPK, repository.domainSeparatorV4().toTypedDataHash(bytecodeHash)); - bc.authorSignature = abi.encodePacked(r, s, v); - - vm.prank(author); - repository.uploadBytecode(bc); - - // Mark as system contract to auto-approve - vm.prank(owner); - repository.allowSystemContract(bytecodeHash); - - vm.expectRevert( - abi.encodeWithSelector( - IBytecodeRepository.BytecodeIsNotAllowedException.selector, _TEST_CONTRACT, _TEST_VERSION - ) - ); - repository.deploy(_TEST_CONTRACT, _TEST_VERSION, "", _TEST_SALT); - } } diff --git a/contracts/test/libraries/Domain.unit.t.sol b/contracts/test/libraries/Domain.unit.t.sol index 76a5350..3b89290 100644 --- a/contracts/test/libraries/Domain.unit.t.sol +++ b/contracts/test/libraries/Domain.unit.t.sol @@ -7,31 +7,84 @@ import {Test} from "forge-std/Test.sol"; import {Domain} from "../../libraries/Domain.sol"; contract DomainUnitTest is Test { - /// @notice Test extracting domain from string with domain separator - function test_Domain_01_extracts_domain_with_separator() public pure { - string memory input = "test::name"; - string memory domain = Domain.extractDomain(input); - assertEq(domain, "test"); + /// @notice Tests extracting domain from various inputs + function test_U_DOM_01_extracts_domain() public pure { + assertEq(Domain.extractDomain("test::name"), "test"); // With separator + assertEq(Domain.extractDomain("test"), "test"); // Without separator + assertEq(Domain.extractDomain(""), ""); // Empty string + assertEq(Domain.extractDomain("test::sub::name"), "test"); // Multiple separators } - /// @notice Test extracting domain from string without domain separator - function test_Domain_02_extracts_full_domain_without_separator() public pure { - string memory input = "test"; - string memory domain = Domain.extractDomain(input); - assertEq(domain, "test"); + /// @notice Tests extracting postfix from various inputs + function test_U_DOM_02_extracts_postfix() public pure { + assertEq(Domain.extractPostfix("test::name"), "name"); // With separator + assertEq(Domain.extractPostfix("test"), bytes32(0)); // Without separator + assertEq(Domain.extractDomain(""), ""); // Empty string + assertEq(Domain.extractPostfix("test::sub::name"), "sub::name"); // Multiple separators } - /// @notice Test extracting domain from empty string - function test_Domain_03_extracts_empty_domain_from_empty_string() public pure { - string memory input = ""; - string memory domain = Domain.extractDomain(input); - assertEq(domain, ""); + /// @notice Tests getting contract type with various inputs + function test_U_DOM_03_gets_contract_type() public pure { + assertEq(Domain.getContractType("test", "name"), "test::name"); // With domain and postfix + assertEq(Domain.getContractType("test", bytes32(0)), "test"); // With domain only + assertEq(Domain.getContractType(bytes32(0), "name"), "::name"); // With postfix only } - /// @notice Test extracting domain with multiple separators - function test_Domain_04_extracts_first_domain_with_multiple_separators() public pure { - string memory input = "test::sub::name"; - string memory domain = Domain.extractDomain(input); - assertEq(domain, "test"); + /// @notice Tests domain validation + function test_U_DOM_04_validates_domain() public pure { + // Valid domains + assertTrue(Domain.isValidDomain("test")); + assertTrue(Domain.isValidDomain("test123")); + assertTrue(Domain.isValidDomain("test_type")); + assertTrue(Domain.isValidDomain("testType")); + assertTrue(Domain.isValidDomain("123")); + assertTrue(Domain.isValidDomain("_")); + + // Invalid domains + assertFalse(Domain.isValidDomain("")); // empty + assertFalse(Domain.isValidDomain("test space")); // spaces + assertFalse(Domain.isValidDomain("test$")); // symbols + assertFalse(Domain.isValidDomain(unicode"тест")); // unicode + } + + /// @notice Tests postfix validation + function test_U_DOM_05_validates_postfix() public pure { + // Valid postfixes + assertTrue(Domain.isValidPostfix("name")); + assertTrue(Domain.isValidPostfix("name456")); + assertTrue(Domain.isValidPostfix("name_test")); + assertTrue(Domain.isValidPostfix("nameTest")); + assertTrue(Domain.isValidPostfix("456")); + assertTrue(Domain.isValidPostfix("_")); + assertTrue(Domain.isValidPostfix(bytes32(0))); // empty postfix is valid + + // Invalid postfixes + assertFalse(Domain.isValidPostfix("name space")); // spaces + assertFalse(Domain.isValidPostfix("name#")); // symbols + assertFalse(Domain.isValidPostfix(unicode"имя")); // unicode + } + + /// @notice Tests contract type validation + function test_U_DOM_06_validates_contract_type() public pure { + // Valid contract types + assertTrue(Domain.isValidContractType("test::name")); // with postfix + assertTrue(Domain.isValidContractType("test")); // without postfix + assertTrue(Domain.isValidContractType("test123::name456")); // with numbers + assertTrue(Domain.isValidContractType("test_type::name_test")); // with underscores + assertTrue(Domain.isValidContractType("testType::nameTest")); // mixed case + assertTrue(Domain.isValidContractType("123::456")); // only numbers + assertTrue(Domain.isValidContractType("_::_")); // only underscores + assertTrue(Domain.isValidContractType("a::b")); // single chars + + // Invalid contract types + assertFalse(Domain.isValidContractType("")); // empty + assertFalse(Domain.isValidContractType("test::sub::name")); // double nested + assertFalse(Domain.isValidContractType("test::")); // empty postfix after separator + assertFalse(Domain.isValidContractType("test space::name")); // invalid domain + assertFalse(Domain.isValidContractType("test::name space")); // invalid postfix + assertFalse(Domain.isValidContractType("test$::name")); // invalid domain symbol + assertFalse(Domain.isValidContractType("test::name#")); // invalid postfix symbol + assertFalse(Domain.isValidContractType(unicode"тест::name")); // invalid domain unicode + assertFalse(Domain.isValidContractType(unicode"test::имя")); // invalid postfix unicode } } diff --git a/contracts/traits/DeployerTrait.sol b/contracts/traits/DeployerTrait.sol index f7b9ce4..7853cff 100644 --- a/contracts/traits/DeployerTrait.sol +++ b/contracts/traits/DeployerTrait.sol @@ -10,6 +10,7 @@ import {IBytecodeRepository} from "../interfaces/IBytecodeRepository.sol"; import {IDeployerTrait} from "../interfaces/base/IDeployerTrait.sol"; import {AP_BYTECODE_REPOSITORY, NO_VERSION_CONTROL} from "../libraries/ContractLiterals.sol"; +import {Domain} from "../libraries/Domain.sol"; abstract contract DeployerTrait is IDeployerTrait { using LibString for string; @@ -32,8 +33,7 @@ abstract contract DeployerTrait is IDeployerTrait { } function _getContractType(bytes32 domain, bytes32 postfix) internal pure returns (bytes32) { - if (postfix == 0) return domain; - return string.concat(domain.fromSmallString(), "::", postfix.fromSmallString()).toSmallString(); + return Domain.getContractType(domain, postfix); } function _deploy(bytes32 contractType, uint256 version, bytes memory constructorParams, bytes32 salt) From 201f2bccbead7a4e7dbedda94225691bde851912 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Thu, 27 Feb 2025 21:33:12 +0200 Subject: [PATCH 4/5] feat: solve some `CrossChainMultisig` issues - Recovery mode message is now chain-specific - In recovery mode, self-calls are not skipped - `disableRecoveryMode` must be the only call in the batch --- contracts/global/CrossChainMultisig.sol | 59 +-- contracts/interfaces/ICrossChainMultisig.sol | 4 +- contracts/interfaces/Types.sol | 1 + .../test/global/CrossChainMultisig.unit.t.sol | 343 +++++++++--------- 4 files changed, 205 insertions(+), 202 deletions(-) diff --git a/contracts/global/CrossChainMultisig.sol b/contracts/global/CrossChainMultisig.sol index e5d9700..0554c14 100644 --- a/contracts/global/CrossChainMultisig.sol +++ b/contracts/global/CrossChainMultisig.sol @@ -45,7 +45,8 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh keccak256("CompactBatch(string name,bytes32 batchHash,bytes32 prevHash)"); /// @notice Recovery mode typehash - bytes32 public constant override RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(bytes32 startingBatchHash)"); + bytes32 public constant override RECOVERY_MODE_TYPEHASH = + keccak256("RecoveryMode(uint256 chainId,bytes32 startingBatchHash)"); /// @notice Confirmation threshold uint8 public override confirmationThreshold; @@ -139,8 +140,13 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh } /// @notice Computes struct hash for recovery mode - function computeRecoveryModeHash(bytes32 startingBatchHash) public pure override returns (bytes32) { - return keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, startingBatchHash)); + function computeRecoveryModeHash(uint256 chainId, bytes32 startingBatchHash) + public + pure + override + returns (bytes32) + { + return keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, chainId, startingBatchHash)); } // ---------- // @@ -175,6 +181,7 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh /// @notice Allows Gearbox DAO to submit a new batch on Ethereum Mainnet /// @dev Can only be executed by Gearbox DAO on Ethereum Mainnet + /// @dev If batch contains `disableRecoveryMode` self-call, it must be its only call /// @dev Reverts if `prevHash` is not the hash of the last executed batch /// @dev Reverts if `calls` is empty or contains local self-calls function submitBatch(string calldata name, CrossChainCall[] calldata calls, bytes32 prevHash) @@ -225,6 +232,7 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh /// @dev In the current implementation, signers are trusted not to deviate and only sign batches /// submitted by Gearbox DAO on Ethereum Mainnet. In future versions, DAO decisions will be /// propagated to other chains using bridges or `L1SLOAD`. + /// @dev If batch contains `disableRecoveryMode` self-call, it must be its only call /// @dev Reverts if batch's `prevHash` is not the hash of the last executed batch /// @dev Reverts if batch is empty or contains local self-calls /// @dev Reverts if signatures have duplicates or the number of valid signatures is insufficient @@ -241,28 +249,31 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh } /// @dev Ensures that batch is connected to the last executed batch, is non-empty and contains no local self-calls + /// @dev If batch contains `disableRecoveryMode` self-call, it must be its only call function _verifyBatch(CrossChainCall[] memory calls, bytes32 prevHash) internal view { if (prevHash != lastBatchHash()) revert InvalidPrevHashException(); uint256 len = calls.length; if (len == 0) revert InvalidBatchException(); for (uint256 i; i < len; ++i) { - if (calls[i].chainId != 0 && calls[i].target == address(this)) { - revert InvalidBatchException(); + if (calls[i].target == address(this)) { + if (calls[i].chainId != 0) revert InvalidBatchException(); + if (bytes4(calls[i].callData) == ICrossChainMultisig.disableRecoveryMode.selector && len != 1) { + revert InvalidBatchException(); + } } } } /// @dev Executes a batch of calls skipping local calls from other chains, updates the last executed batch hash - /// @dev In recovery mode, simply updates the last executed batch hash, unless the first call is `disableRecoveryMode` + /// @dev In recovery mode, only self-calls are executed function _executeBatch(CrossChainCall[] memory calls, bytes32 batchHash) internal { - if (!_isRecovery(calls[0])) { - uint256 len = calls.length; - for (uint256 i; i < len; ++i) { - uint256 chainId = calls[i].chainId; - if (chainId == 0 || chainId == block.chainid) { - calls[i].target.functionCall(calls[i].callData, "Call execution failed"); - } + uint256 len = calls.length; + for (uint256 i; i < len; ++i) { + if (isRecoveryModeEnabled && calls[i].target != address(this)) continue; + uint256 chainId = calls[i].chainId; + if (chainId == 0 || chainId == block.chainid) { + calls[i].target.functionCall(calls[i].callData, "Call execution failed"); } } _executedBatchHashes.push(batchHash); @@ -346,7 +357,7 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh // RECOVERY MODE // // ------------- // - /// @notice Enables recovery mode, in which calls execution is skipped + /// @notice If `message.chainId` matches current chain, enables recovery mode, in which only self-calls are executed /// @dev Can only be executed outside Ethereum Mainnet /// @dev Reverts if starting batch of recovery mode is not the last executed batch /// @dev Reverts if the number of signatures is insufficient @@ -356,10 +367,10 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh onlyNotOnMainnet nonReentrant { - if (isRecoveryModeEnabled) return; + if (isRecoveryModeEnabled || message.chainId != block.chainid) return; if (message.startingBatchHash != lastBatchHash()) revert InvalidRecoveryModeMessageException(); - bytes32 digest = _hashTypedDataV4(computeRecoveryModeHash(message.startingBatchHash)); + bytes32 digest = _hashTypedDataV4(computeRecoveryModeHash(message.chainId, message.startingBatchHash)); uint256 validSignatures = _verifySignatures(message.signatures, digest); if (validSignatures < confirmationThreshold) revert InsufficientNumberOfSignaturesException(); @@ -367,21 +378,11 @@ contract CrossChainMultisig is EIP712Mainnet, Ownable, ReentrancyGuard, ICrossCh emit EnableRecoveryMode(message.startingBatchHash); } - /// @notice Disables recovery mode + /// @notice If `chainId` matches current chain, disables recovery mode /// @dev Can only be executed by the contract itself - function disableRecoveryMode() external override onlySelf { - if (!isRecoveryModeEnabled) return; + function disableRecoveryMode(uint256 chainId) external override onlySelf { + if (!isRecoveryModeEnabled || chainId != block.chainid) return; isRecoveryModeEnabled = false; emit DisableRecoveryMode(); } - - /// @dev Whether the batch should be skipped due to recovery mode - function _isRecovery(CrossChainCall memory call0) internal view returns (bool) { - if (!isRecoveryModeEnabled) return false; - - return !( - (call0.chainId == 0) && (call0.target == address(this)) - && (bytes4(call0.callData) == ICrossChainMultisig.disableRecoveryMode.selector) - ); - } } diff --git a/contracts/interfaces/ICrossChainMultisig.sol b/contracts/interfaces/ICrossChainMultisig.sol index c1deedf..07ee9be 100644 --- a/contracts/interfaces/ICrossChainMultisig.sol +++ b/contracts/interfaces/ICrossChainMultisig.sol @@ -56,7 +56,7 @@ interface ICrossChainMultisig is IVersion { external view returns (bytes32); - function computeRecoveryModeHash(bytes32 startingBatchHash) external view returns (bytes32); + function computeRecoveryModeHash(uint256 chainId, bytes32 startingBatchHash) external view returns (bytes32); // ---------- // // GOVERNANCE // @@ -88,5 +88,5 @@ interface ICrossChainMultisig is IVersion { function isRecoveryModeEnabled() external view returns (bool); function enableRecoveryMode(SignedRecoveryModeMessage calldata message) external; - function disableRecoveryMode() external; + function disableRecoveryMode(uint256 chainId) external; } diff --git a/contracts/interfaces/Types.sol b/contracts/interfaces/Types.sol index 4c3514b..d381c97 100644 --- a/contracts/interfaces/Types.sol +++ b/contracts/interfaces/Types.sol @@ -83,6 +83,7 @@ struct SignedBatch { } struct SignedRecoveryModeMessage { + uint256 chainId; bytes32 startingBatchHash; bytes[] signatures; } diff --git a/contracts/test/global/CrossChainMultisig.unit.t.sol b/contracts/test/global/CrossChainMultisig.unit.t.sol index ecaa530..5bd85ba 100644 --- a/contracts/test/global/CrossChainMultisig.unit.t.sol +++ b/contracts/test/global/CrossChainMultisig.unit.t.sol @@ -20,7 +20,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { address owner; bytes32 COMPACT_BATCH_TYPEHASH = keccak256("CompactBatch(string name,bytes32 batchHash,bytes32 prevHash)"); - bytes32 RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(bytes32 startingBatchHash)"); + bytes32 RECOVERY_MODE_TYPEHASH = keccak256("RecoveryMode(uint256 chainId,bytes32 startingBatchHash)"); function setUp() public { // Setup initial signers @@ -56,8 +56,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { return abi.encodePacked(r, s, v); } - /// @dev U:[SM-1]: Initial state is correct - function test_CCG_01_InitialState() public { + /// @notice U:[CCM-1]: Initial state is correct + function test_U_CCM_01_InitialState() public { assertEq(multisig.confirmationThreshold(), THRESHOLD); assertEq(multisig.lastBatchHash(), bytes32(0)); assertEq(multisig.owner(), owner); @@ -84,8 +84,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { new CrossChainMultisigHarness(signers, THRESHOLD, owner); } - /// @dev U:[SM-2]: Access modifiers work correctly - function test_CCG_02_AccessModifiers() public { + /// @notice U:[CCM-2]: Access modifiers work correctly + function test_U_CCM_02_AccessModifiers() public { // Test onlyOnMainnet modifier vm.chainId(5); // Set to non-mainnet chain vm.startPrank(owner); @@ -116,7 +116,7 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.setConfirmationThreshold(3); vm.expectRevert(abi.encodeWithSelector(ICrossChainMultisig.CallerIsNotSelfException.selector, address(this))); - multisig.disableRecoveryMode(); + multisig.disableRecoveryMode(0); // Test onlyOwner modifier vm.prank(makeAddr("notOwner")); @@ -124,8 +124,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.submitBatch("test", calls, bytes32(0)); } - /// @dev U:[SM-3]: Submit batch works correctly - function test_CCG_03_SubmitBatch() public { + /// @notice U:[CCM-3]: Submit batch works correctly + function test_U_CCM_03_SubmitBatch() public { vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); @@ -154,7 +154,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(batch.signatures.length, 0); } - function test_CCG_04_RevertOnInvalidPrevHash() public { + /// @notice U:[CCM-4]: Reverts when submitting batch with invalid prev hash + function test_U_CCM_04_revert_on_invalid_prev_hash() public { vm.chainId(1); vm.startPrank(owner); @@ -165,7 +166,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.submitBatch("test", calls, bytes32(uint256(1))); // Invalid prevHash } - function test_CCG_05_RevertOnEmptyCalls() public { + /// @notice U:[CCM-5]: Reverts when submitting empty batch + function test_U_CCM_05_revert_on_empty_calls() public { vm.chainId(1); vm.startPrank(owner); @@ -175,8 +177,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.submitBatch("test", calls, bytes32(0)); } - /// @dev U:[SM-6]: Sign batch works correctly with single signature - function test_CCG_06_SignBatch() public { + /// @notice U:[CCM-6]: Sign batch works correctly with single signature + function test_U_CCM_06_SignBatch() public { vm.chainId(1); // Set to mainnet // Submit batch @@ -208,8 +210,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(multisig.lastBatchHash(), bytes32(0)); } - /// @dev U:[SM-7]: Sign batch reverts when signing with invalid signature - function test_CCG_07_SignBatchInvalidSignature() public { + /// @notice U:[CCM-7]: Sign batch reverts when signing with invalid signature + function test_U_CCM_07_SignBatchInvalidSignature() public { vm.chainId(1); // Submit batch @@ -226,8 +228,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.signBatch(batchHash, invalidSig); } - /// @dev U:[SM-8]: Sign batch reverts when signing non-existent batch - function test_CCG_08_SignBatchNonExistentBatch() public { + /// @notice U:[CCM-8]: Sign batch reverts when signing non-existent batch + function test_U_CCM_08_SignBatchNonExistentBatch() public { vm.chainId(1); // Set last batch hash to 1, to avoid ambiguity that default value of 0 is a valid batch hash @@ -242,8 +244,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.signBatch(nonExistentHash, signature); } - /// @dev U:[SM-9]: Sign batch reverts when same signer signs twice - function test_CCG_09_SignBatchDuplicateSigner() public { + /// @notice U:[CCM-9]: Sign batch reverts when same signer signs twice + function test_U_CCM_09_SignBatchDuplicateSigner() public { vm.chainId(1); // Submit batch @@ -266,8 +268,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.signBatch(batchHash, signature); } - /// @dev U:[SM-10]: Sign and execute proposal works correctly - function test_CCG_11_SignAndExecuteProposal() public { + /// @notice U:[CCM-10]: Sign and execute proposal works correctly + function test_U_CCM_10_SignAndExecuteProposal() public { vm.chainId(1); // Set to mainnet // Submit proposal @@ -304,40 +306,48 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(multisig.getExecutedBatchHashes()[0], batchHash, "executedBatchHashes"); } - /// @dev U:[SM-12]: _verifyBatch reverts if prevHash doesn't match lastBatchHash - function test_CCG_12_VerifyBatchInvalidPrevHash() public { - CrossChainCall[] memory calls = new CrossChainCall[](1); - calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); + /// @notice U:[CCM-11]: Batch validation works correctly + function test_U_CCM_11_BatchValidation() public { + vm.chainId(5); // Set to non-mainnet chain - bytes32 invalidPrevHash = keccak256("invalid"); - vm.expectRevert(ICrossChainMultisig.InvalidPrevHashException.selector); - multisig.exposed_verifyBatch(calls, invalidPrevHash); - } + // Test empty batch + CrossChainCall[] memory emptyCalls = new CrossChainCall[](0); + vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); + multisig.executeBatch( + SignedBatch({name: "test", calls: emptyCalls, prevHash: bytes32(0), signatures: new bytes[](0)}) + ); - /// @dev U:[SM-13]: _verifyBatch reverts if calls array is empty - function test_CCG_13_VerifyBatchEmptyCalls() public { - CrossChainCall[] memory calls = new CrossChainCall[](0); + // Test invalid prev hash + CrossChainCall[] memory calls = new CrossChainCall[](1); + calls[0] = CrossChainCall({chainId: 5, target: makeAddr("target"), callData: hex"1234"}); + vm.expectRevert(ICrossChainMultisig.InvalidPrevHashException.selector); + multisig.executeBatch( + SignedBatch({name: "test", calls: calls, prevHash: bytes32(uint256(1)), signatures: new bytes[](0)}) + ); + // Test local self-call + calls[0] = CrossChainCall({chainId: 5, target: address(multisig), callData: hex"1234"}); vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); - multisig.exposed_verifyBatch(calls, bytes32(0)); - } + multisig.executeBatch( + SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](0)}) + ); - /// @dev U:[SM-14]: _verifyBatch reverts if trying to call self on other chain - function test_CCG_14_VerifyBatchSelfCallOtherChain() public { - CrossChainCall[] memory calls = new CrossChainCall[](1); - // Try to call the multisig contract itself on another chain - calls[0] = CrossChainCall({ - chainId: 5, // Goerli chain ID + // Test disableRecoveryMode not being the only call + CrossChainCall[] memory mixedCalls = new CrossChainCall[](2); + mixedCalls[0] = CrossChainCall({ + chainId: 0, target: address(multisig), - callData: hex"1234" + callData: abi.encodeWithSelector(ICrossChainMultisig.disableRecoveryMode.selector, block.chainid) }); - + mixedCalls[1] = CrossChainCall({chainId: 5, target: makeAddr("target"), callData: hex"1234"}); vm.expectRevert(ICrossChainMultisig.InvalidBatchException.selector); - multisig.exposed_verifyBatch(calls, bytes32(0)); + multisig.executeBatch( + SignedBatch({name: "test", calls: mixedCalls, prevHash: bytes32(0), signatures: new bytes[](0)}) + ); } - /// @dev U:[SM-15]: _verifyBatch succeeds with valid calls - function test_CCG_15_VerifyBatchValidCalls() public view { + /// @notice U:[CCM-12]: _verifyBatch succeeds with valid calls + function test_U_CCM_12_VerifyBatchValidCalls() public view { CrossChainCall[] memory calls = new CrossChainCall[](3); // Valid call on same chain @@ -353,8 +363,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.exposed_verifyBatch(calls, bytes32(0)); } - /// @dev U:[SM-16]: _verifySignatures returns 0 for empty signatures array - function test_CCG_16_VerifySignaturesEmptyArray() public view { + /// @notice U:[CCM-13]: _verifySignatures returns 0 for empty signatures array + function test_U_CCM_13_VerifySignaturesEmptyArray() public view { bytes[] memory signatures = new bytes[](0); bytes32 batchHash = keccak256("test"); @@ -362,8 +372,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(validCount, 0); } - /// @dev U:[SM-17]: _verifySignatures correctly counts valid signatures - function test_CCG_17_VerifySignaturesValidSignatures() public { + /// @notice U:[CCM-14]: _verifySignatures correctly counts valid signatures + function test_U_CCM_14_VerifySignaturesValidSignatures() public { vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); @@ -384,8 +394,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(validCount, 2); } - /// @dev U:[SM-18]: _verifySignatures ignores invalid signatures - function test_CCG_18_VerifySignaturesInvalidSignatures() public { + /// @notice U:[CCM-15]: _verifySignatures ignores invalid signatures + function test_U_CCM_15_VerifySignaturesInvalidSignatures() public { vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); @@ -407,9 +417,9 @@ contract CrossChainMultisigTest is Test, SignatureHelper { uint256 validCount = multisig.exposed_verifySignatures(signatures, _getDigest(structHash)); assertEq(validCount, 1); } - /// @dev U:[SM-19]: _verifySignatures reverts with DuplicateSignatureException on duplicate signatures from same signer - function test_CCG_19_VerifySignaturesDuplicateSigner() public { + /// @notice U:[CCM-16]: _verifySignatures reverts with DuplicateSignatureException on duplicate signatures from same signer + function test_U_CCM_16_VerifySignaturesDuplicateSigner() public { vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); @@ -432,8 +442,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.exposed_verifySignatures(signatures, digest); } - /// @dev U:[SM-20]: _verifySignatures ignores signatures from non-signers - function test_CCG_20_VerifySignaturesNonSigner() public { + /// @notice U:[CCM-17]: _verifySignatures ignores signatures from non-signers + function test_U_CCM_17_VerifySignaturesNonSigner() public { vm.chainId(1); // Set to mainnet CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = CrossChainCall({chainId: 1, target: address(0x123), callData: hex"1234"}); @@ -456,8 +466,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { assertEq(validCount, 1); } - /// @dev U:[SM-21]: _verifySignatures reverts on malformed signatures - function test_CCG_21_VerifySignaturesMalformedSignature() public { + /// @notice U:[CCM-18]: _verifySignatures reverts on malformed signatures + function test_U_CCM_18_VerifySignaturesMalformedSignature() public { bytes32 batchHash = keccak256("test"); bytes[] memory signatures = new bytes[](2); @@ -468,14 +478,27 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.exposed_verifySignatures(signatures, batchHash); } - /// @dev U:[SM-22]: Recovery mode can be enabled with valid signatures - function test_CCG_22_EnableRecoveryMode() public { + /// @notice U:[CCM-19]: Cannot reduce signers below threshold + function test_U_CCM_19_CannotReduceSignersBelowThreshold() public { + vm.prank(address(multisig)); + multisig.removeSigner(signers[0]); + + vm.expectRevert(ICrossChainMultisig.InvalidConfirmationThresholdException.selector); + vm.prank(address(multisig)); + multisig.removeSigner(signers[1]); + } + + /// @notice U:[CCM-20]: Recovery mode can be enabled with valid signatures + function test_U_CCM_20_EnableRecoveryMode() public { vm.chainId(5); // Set to non-mainnet chain - address payable calledContract = payable(new GeneralMock()); + address target = makeAddr("target"); + vm.etch(target, hex"ff"); // Put some code there to make call possible + vm.mockCall(target, hex"1234", ""); + // First execute a batch to have non-zero lastBatchHash CrossChainCall[] memory calls = new CrossChainCall[](1); - calls[0] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"1234"}); + calls[0] = CrossChainCall({chainId: 5, target: target, callData: hex"1234"}); SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); @@ -489,7 +512,8 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.executeBatch(batch); - bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, batchHash)); + // Now enable recovery mode + bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, block.chainid, batchHash)); bytes[] memory signatures = new bytes[](2); signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); @@ -498,151 +522,134 @@ contract CrossChainMultisigTest is Test, SignatureHelper { vm.expectEmit(true, false, false, false); emit ICrossChainMultisig.EnableRecoveryMode(batchHash); - multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); + multisig.enableRecoveryMode( + SignedRecoveryModeMessage({chainId: block.chainid, startingBatchHash: batchHash, signatures: signatures}) + ); assertTrue(multisig.isRecoveryModeEnabled()); } - /// @dev U:[SM-23]: Recovery mode skips batch execution except for lastBatchHash update - function test_CCG_23_RecoveryModeSkipsExecution() public { + /// @notice U:[CCM-21]: Recovery mode skips non-self calls during execution + function test_U_CCM_21_RecoveryModeSkipsExecution() public { vm.chainId(5); // Set to non-mainnet chain - address payable calledContract = payable(new GeneralMock()); + // Setup and enable recovery mode first + bytes32 lastHash = _setupRecoveryMode(); + assertTrue(multisig.isRecoveryModeEnabled()); - // First submit and execute a batch to have non-zero lastBatchHash - CrossChainCall[] memory calls = new CrossChainCall[](1); - calls[0] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"1234"}); + address target = makeAddr("target"); + vm.mockCallRevert(target, hex"1234", ""); // This call should be skipped + + // Create batch with both self and external calls + CrossChainCall[] memory calls = new CrossChainCall[](2); + calls[0] = CrossChainCall({ + chainId: 5, + target: target, + callData: hex"1234" // This should be skipped + }); + calls[1] = CrossChainCall({ + chainId: 0, + target: address(multisig), + callData: abi.encodeWithSelector(ICrossChainMultisig.setConfirmationThreshold.selector, 3) + }); SignedBatch memory batch = - SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); + SignedBatch({name: "test", calls: calls, prevHash: lastHash, signatures: new bytes[](2)}); - bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, lastHash); bytes32 structHash = - keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, lastHash)); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); + // Execute batch and verify only self-call was executed multisig.executeBatch(batch); - - // Enable recovery mode - bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, batchHash)); - bytes[] memory signatures = new bytes[](2); - signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); - signatures[1] = _signBatchHash(signer1PrivateKey, recoveryHash); - - multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); - - calledContract = payable(new GeneralMock()); - - CrossChainCall[] memory calls2 = new CrossChainCall[](1); - calls2[0] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"5678"}); - - SignedBatch memory batch2 = - SignedBatch({name: "test2", calls: calls2, prevHash: batchHash, signatures: new bytes[](2)}); - - bytes32 batchHash2 = multisig.computeBatchHash("test2", calls2, batchHash); - bytes32 structHash2 = - keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); - - batch2.signatures[0] = _signBatchHash(signer0PrivateKey, structHash2); - batch2.signatures[1] = _signBatchHash(signer1PrivateKey, structHash2); - - multisig.executeBatch(batch2); - - // Verify lastBatchHash was updated but call wasn't executed - assertEq(multisig.lastBatchHash(), batchHash2); - assertTrue(multisig.isRecoveryModeEnabled()); - assertEq(GeneralMock(calledContract).data().length, 0); + assertEq(multisig.confirmationThreshold(), 3); // Self-call executed } - /// @dev U:[SM-24]: Recovery mode can be disabled through a batch with correct first call - function test_CCG_24_DisableRecoveryMode() public { + /// @notice U:[CCM-22]: Recovery mode can be disabled through dedicated batch + function test_U_CCM_22_DisableRecoveryMode() public { vm.chainId(5); // Set to non-mainnet chain - address payable calledContract = payable(new GeneralMock()); + // Setup and enable recovery mode first + bytes32 lastHash = _setupRecoveryMode(); + assertTrue(multisig.isRecoveryModeEnabled()); - // First submit and execute a batch to have non-zero lastBatchHash + // Create batch with single disableRecoveryMode call CrossChainCall[] memory calls = new CrossChainCall[](1); - calls[0] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"1234"}); + calls[0] = CrossChainCall({ + chainId: 0, + target: address(multisig), + callData: abi.encodeWithSelector(ICrossChainMultisig.disableRecoveryMode.selector, block.chainid) + }); SignedBatch memory batch = - SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); + SignedBatch({name: "test", calls: calls, prevHash: lastHash, signatures: new bytes[](2)}); - bytes32 batchHash = multisig.computeBatchHash("test", calls, bytes32(0)); + bytes32 batchHash = multisig.computeBatchHash("test", calls, lastHash); bytes32 structHash = - keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, bytes32(0))); + keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test")), batchHash, lastHash)); batch.signatures[0] = _signBatchHash(signer0PrivateKey, structHash); batch.signatures[1] = _signBatchHash(signer1PrivateKey, structHash); + vm.expectEmit(false, false, false, true); + emit ICrossChainMultisig.DisableRecoveryMode(); + multisig.executeBatch(batch); + assertFalse(multisig.isRecoveryModeEnabled()); + } + + /// @notice U:[CCM-23]: Recovery mode cannot be enabled on mainnet + function test_U_CCM_23_EnableRecoveryModeOnMainnet() public { + vm.chainId(1); // Set to mainnet + + bytes32 batchHash = bytes32(uint256(1)); + bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, block.chainid, batchHash)); - // Enable recovery mode - bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, batchHash)); bytes[] memory signatures = new bytes[](2); signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); signatures[1] = _signBatchHash(signer1PrivateKey, recoveryHash); - multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); - - calledContract = payable(new GeneralMock()); - - // Now submit a batch that disables recovery mode - CrossChainCall[] memory calls2 = new CrossChainCall[](2); - calls2[0] = CrossChainCall({ - chainId: 0, - target: address(multisig), - callData: abi.encodeWithSelector(ICrossChainMultisig.disableRecoveryMode.selector) - }); - calls2[1] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"1234"}); - - SignedBatch memory batch2 = - SignedBatch({name: "test2", calls: calls2, prevHash: batchHash, signatures: new bytes[](2)}); - - bytes32 batchHash2 = multisig.computeBatchHash("test2", calls2, batchHash); - bytes32 structHash2 = - keccak256(abi.encode(COMPACT_BATCH_TYPEHASH, keccak256(bytes("test2")), batchHash2, batchHash)); - - batch2.signatures[0] = _signBatchHash(signer0PrivateKey, structHash2); - batch2.signatures[1] = _signBatchHash(signer1PrivateKey, structHash2); - - vm.expectEmit(false, false, false, true); - emit ICrossChainMultisig.DisableRecoveryMode(); - - multisig.executeBatch(batch2); - - // Verify recovery mode was disabled and both calls were executed - assertFalse(multisig.isRecoveryModeEnabled()); - assertEq(multisig.lastBatchHash(), batchHash2); - assertEq(GeneralMock(calledContract).data(), hex"1234"); + vm.expectRevert(ICrossChainMultisig.CantBeExecutedOnCurrentChainException.selector); + multisig.enableRecoveryMode( + SignedRecoveryModeMessage({chainId: block.chainid, startingBatchHash: batchHash, signatures: signatures}) + ); } - /// @dev U:[SM-25]: Recovery mode cannot be enabled with invalid starting batch hash - function test_CCG_25_EnableRecoveryModeInvalidStartingHash() public { + /// @notice U:[CCM-24]: Recovery mode message must match current chain + function test_U_CCM_24_EnableRecoveryModeWrongChain() public { vm.chainId(5); // Set to non-mainnet chain - // Try to enable recovery mode with wrong starting hash - bytes32 wrongHash = keccak256("wrong"); - bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, wrongHash)); + bytes32 batchHash = bytes32(uint256(1)); + // Create recovery message for wrong chain + bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, uint256(137), batchHash)); bytes[] memory signatures = new bytes[](2); signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); signatures[1] = _signBatchHash(signer1PrivateKey, recoveryHash); - vm.expectRevert(ICrossChainMultisig.InvalidRecoveryModeMessageException.selector); - multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: wrongHash, signatures: signatures})); + // Should silently return without enabling recovery mode + multisig.enableRecoveryMode( + SignedRecoveryModeMessage({ + chainId: 137, // Different chain + startingBatchHash: batchHash, + signatures: signatures + }) + ); + assertFalse(multisig.isRecoveryModeEnabled()); } - /// @dev U:[SM-26]: Recovery mode cannot be enabled with insufficient signatures - function test_CCG_26_EnableRecoveryModeInsufficientSignatures() public { - vm.chainId(5); // Set to non-mainnet chain + /// Helper function to setup recovery mode + function _setupRecoveryMode() internal returns (bytes32) { + address target = makeAddr("target"); + vm.etch(target, hex"ff"); // Put some code there to make call possible + vm.mockCall(target, hex"1234", ""); - address payable calledContract = payable(new GeneralMock()); - - // First submit and execute a batch to have non-zero lastBatchHash + // First execute a batch to have non-zero lastBatchHash CrossChainCall[] memory calls = new CrossChainCall[](1); - calls[0] = CrossChainCall({chainId: 5, target: calledContract, callData: hex"1234"}); + calls[0] = CrossChainCall({chainId: 5, target: target, callData: hex"1234"}); SignedBatch memory batch = SignedBatch({name: "test", calls: calls, prevHash: bytes32(0), signatures: new bytes[](2)}); @@ -656,22 +663,16 @@ contract CrossChainMultisigTest is Test, SignatureHelper { multisig.executeBatch(batch); - // Try to enable recovery mode with only one signature - bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, batchHash)); - - bytes[] memory signatures = new bytes[](1); + // Enable recovery mode + bytes32 recoveryHash = keccak256(abi.encode(RECOVERY_MODE_TYPEHASH, block.chainid, batchHash)); + bytes[] memory signatures = new bytes[](2); signatures[0] = _signBatchHash(signer0PrivateKey, recoveryHash); + signatures[1] = _signBatchHash(signer1PrivateKey, recoveryHash); - vm.expectRevert(ICrossChainMultisig.InsufficientNumberOfSignaturesException.selector); - multisig.enableRecoveryMode(SignedRecoveryModeMessage({startingBatchHash: batchHash, signatures: signatures})); - } - - function test_CCG_27_cannot_reduce_signers_below_threshold() public { - vm.prank(address(multisig)); - multisig.removeSigner(signers[0]); + multisig.enableRecoveryMode( + SignedRecoveryModeMessage({chainId: block.chainid, startingBatchHash: batchHash, signatures: signatures}) + ); - vm.expectRevert(ICrossChainMultisig.InvalidConfirmationThresholdException.selector); - vm.prank(address(multisig)); - multisig.removeSigner(signers[1]); + return batchHash; } } From 729e1baf8267094ba4b101b1aa7d6f3781e4cb94 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Sun, 2 Mar 2025 13:00:59 +0200 Subject: [PATCH 5/5] fix: minor fixes and proper tests setup (#37) --- contracts/global/BytecodeRepository.sol | 11 +- contracts/instance/InstanceManager.sol | 18 ++- contracts/instance/PriceFeedStore.sol | 10 +- contracts/interfaces/IBytecodeRepository.sol | 53 +++---- contracts/market/MarketConfigurator.sol | 2 +- .../legacy/MarketConfiguratorLegacy.sol | 5 +- .../configuration/ConfigurationTestHelper.sol | 4 +- .../InterestRateModelConfiguration.unit.t.sol | 4 +- .../test/global/InstanceManager.unit.t.sol | 3 +- .../test/global/PriceFeedStore.unit.t.sol | 40 +----- contracts/test/helpers/GlobalSetup.sol | 132 +++++++++++------- .../test/helpers/InstanceManagerHelper.sol | 11 ++ contracts/test/suite/NewChainDeploySuite.sol | 4 +- lib/@gearbox-protocol/core-v3 | 2 +- wagmi.config.js | 4 - 15 files changed, 157 insertions(+), 146 deletions(-) diff --git a/contracts/global/BytecodeRepository.sol b/contracts/global/BytecodeRepository.sol index 876ebd6..dee7d20 100644 --- a/contracts/global/BytecodeRepository.sol +++ b/contracts/global/BytecodeRepository.sol @@ -248,7 +248,14 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod source: bytecode.source, authorSignature: bytecode.authorSignature }); - emit UploadBytecode(bytecodeHash, bytecode.contractType, bytecode.version, bytecode.author, bytecode.source); + emit UploadBytecode( + bytecodeHash, + bytecode.contractType, + bytecode.version, + bytecode.author, + bytecode.source, + bytecode.authorSignature + ); } // ----------------- // @@ -299,7 +306,7 @@ contract BytecodeRepository is ImmutableOwnableTrait, SanityCheckTrait, IBytecod } } reports.push(auditReport); - emit AuditBytecode(bytecodeHash, auditor, auditReport.reportUrl); + emit AuditBytecode(bytecodeHash, auditor, auditReport.reportUrl, auditReport.signature); } // ----------------- // diff --git a/contracts/instance/InstanceManager.sol b/contracts/instance/InstanceManager.sol index e2d9295..170aaf9 100644 --- a/contracts/instance/InstanceManager.sol +++ b/contracts/instance/InstanceManager.sol @@ -137,7 +137,7 @@ contract InstanceManager is Ownable, IInstanceManager { ? _getLegacyGearStakingAddress() : _deploySystemContract(contractType_, version_); - if (newSystemContract != address(0)) _setAddress(contractType_, newSystemContract, saveVersion); + _setAddress(contractType_, newSystemContract, saveVersion); } /// @notice Allows cross-chain governance to set a global address in the address provider @@ -203,15 +203,12 @@ contract InstanceManager is Ownable, IInstanceManager { } /// @dev Deploys a system contract and returns its address - function _deploySystemContract(bytes32 _contractType, uint256 _version) internal returns (address) { - try ProxyCall(crossChainGovernanceProxy).proxyCall( + function _deploySystemContract(bytes32 contractType_, uint256 version_) internal returns (address) { + bytes memory result = ProxyCall(crossChainGovernanceProxy).proxyCall( address(bytecodeRepository), - abi.encodeCall(BytecodeRepository.deploy, (_contractType, _version, abi.encode(addressProvider), 0)) - ) returns (bytes memory result) { - return abi.decode(result, (address)); - } catch { - return address(0); - } + abi.encodeCall(BytecodeRepository.deploy, (contractType_, version_, abi.encode(addressProvider), 0)) + ); + return abi.decode(result, (address)); } /// @dev Whether there is a legacy instance on this chain @@ -229,7 +226,8 @@ contract InstanceManager is Ownable, IInstanceManager { return 0xe88846b6C85AA67688e453c7eaeeeb40F51e1F0a; } else if (block.chainid == 42161) { return 0xf3599BEfe8E79169Afd5f0b7eb0A1aA322F193D9; + } else { + revert(); } - return address(0); } } diff --git a/contracts/instance/PriceFeedStore.sol b/contracts/instance/PriceFeedStore.sol index 2ee31d8..44c28b3 100644 --- a/contracts/instance/PriceFeedStore.sol +++ b/contracts/instance/PriceFeedStore.sol @@ -236,13 +236,13 @@ contract PriceFeedStore is /// @notice Executes price feed configuration `calls` with owner privileges /// @dev Reverts if caller is not owner /// @dev Reverts if any of call targets is not a known price feed - /// @dev Reverts if any of calls transfers or renounces ownership over price feed + /// @dev Reverts if any of calls renounces ownership over price feed function configurePriceFeeds(Call[] calldata calls) external override onlyOwner { uint256 numCalls = calls.length; for (uint256 i; i < numCalls; ++i) { if (!_knownPriceFeeds.contains(calls[i].target)) revert PriceFeedIsNotKnownException(calls[i].target); bytes4 selector = bytes4(calls[i].callData); - if (selector == Ownable.transferOwnership.selector || selector == Ownable.renounceOwnership.selector) { + if (selector == Ownable.renounceOwnership.selector) { revert ForbiddenConfigurationMethodException(selector); } calls[i].target.functionCall(calls[i].callData); @@ -292,14 +292,12 @@ contract PriceFeedStore is /// @dev Returns whether `priceFeed` is deployed externally or via BCR. /// For latter case, also ensures that price feed is owned by the store. - function _validatePriceFeedDeployment(address priceFeed) internal view returns (bool) { + function _validatePriceFeedDeployment(address priceFeed) internal returns (bool) { if (!IBytecodeRepository(bytecodeRepository).isDeployedFromRepository(priceFeed)) return true; + try Ownable2Step(priceFeed).acceptOwnership() {} catch {} try Ownable(priceFeed).owner() returns (address owner_) { if (owner_ != address(this)) revert PriceFeedIsNotOwnedByStore(priceFeed); - try Ownable2Step(priceFeed).pendingOwner() returns (address pendingOwner_) { - if (pendingOwner_ != address(0)) revert PriceFeedIsNotOwnedByStore(priceFeed); - } catch {} } catch {} return false; diff --git a/contracts/interfaces/IBytecodeRepository.sol b/contracts/interfaces/IBytecodeRepository.sol index 8081b27..261c583 100644 --- a/contracts/interfaces/IBytecodeRepository.sol +++ b/contracts/interfaces/IBytecodeRepository.sol @@ -16,19 +16,24 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { event AddAuditor(address indexed auditor, string name); event AddPublicDomain(bytes32 indexed domain); event AddSystemDomain(bytes32 indexed domain); - event AllowContract(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); - event AuditBytecode(bytes32 indexed bytecodeHash, address indexed auditor, string reportUrl); + event AllowContract(bytes32 indexed bytecodeHash, bytes32 indexed contractType, uint256 indexed version); + event AuditBytecode(bytes32 indexed bytecodeHash, address indexed auditor, string reportUrl, bytes signature); event DeployContract( - bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address contractAddress + bytes32 indexed bytecodeHash, bytes32 indexed contractType, uint256 indexed version, address contractAddress ); - event ForbidContract(bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver); + event ForbidContract(bytes32 indexed bytecodeHash, bytes32 indexed contractType, uint256 indexed version); event ForbidInitCode(bytes32 indexed initCodeHash); event RemoveAuditor(address indexed auditor); - event RemoveContractTypeOwner(bytes32 indexed cType); - event SetContractTypeOwner(bytes32 indexed cType, address indexed owner); + event RemoveContractTypeOwner(bytes32 indexed contractType); + event SetContractTypeOwner(bytes32 indexed contractType, address indexed owner); event SetTokenSpecificPostfix(address indexed token, bytes32 indexed postfix); event UploadBytecode( - bytes32 indexed bytecodeHash, bytes32 indexed cType, uint256 indexed ver, address author, string source + bytes32 indexed bytecodeHash, + bytes32 indexed contractType, + uint256 indexed version, + address author, + string source, + bytes signature ); // ------ // @@ -36,25 +41,25 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { // ------ // error AuditorIsNotApprovedException(address auditor); - error AuthorIsNotContractTypeOwnerException(bytes32 cType, address author); - error BytecodeIsAlreadyAllowedException(bytes32 cType, uint256 ver); + error AuthorIsNotContractTypeOwnerException(bytes32 contractType, address author); + error BytecodeIsAlreadyAllowedException(bytes32 contractType, uint256 version); error BytecodeIsAlreadySignedByAuditorException(bytes32 bytecodeHash, address auditor); - error BytecodeIsNotAllowedException(bytes32 cType, uint256 ver); + error BytecodeIsNotAllowedException(bytes32 contractType, uint256 version); error BytecodeIsNotAuditedException(bytes32 bytecodeHash); error BytecodeIsNotUploadedException(bytes32 bytecodeHash); error CallerIsNotBytecodeAuthorException(address caller); error ContractIsAlreadyDeployedException(address deployedContract); - error ContractTypeIsNotInPublicDomainException(bytes32 cType); + error ContractTypeIsNotInPublicDomainException(bytes32 contractType); error DomainIsAlreadyMarketAsPublicException(bytes32 domain); error DomainIsAlreadyMarketAsSystemException(bytes32 domain); error InitCodeIsForbiddenException(bytes32 initCodeHash); error InvalidAuditorSignatureException(address auditor); error InvalidAuthorSignatureException(address author); error InvalidBytecodeException(bytes32 bytecodeHash); - error InvalidContractTypeException(bytes32 cType); + error InvalidContractTypeException(bytes32 contractType); error InvalidDomainException(bytes32 domain); - error InvalidVersionException(bytes32 cType, uint256 ver); - error VersionNotFoundException(bytes32 cType); + error InvalidVersionException(bytes32 contractType, uint256 version); + error VersionNotFoundException(bytes32 contractType); // --------------- // // EIP-712 GETTERS // @@ -76,13 +81,13 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { function isDeployedFromRepository(address deployedContract) external view returns (bool); function getDeployedContractBytecodeHash(address deployedContract) external view returns (bytes32); function computeAddress( - bytes32 cType, - uint256 ver, + bytes32 contractType, + uint256 version, bytes calldata constructorParams, bytes32 salt, address deployer ) external view returns (address); - function deploy(bytes32 cType, uint256 ver, bytes calldata constructorParams, bytes32 salt) + function deploy(bytes32 contractType, uint256 version, bytes calldata constructorParams, bytes32 salt) external returns (address); @@ -108,11 +113,11 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { // ALLOWING BYTECODE // // ----------------- // - function getAllowedBytecodeHash(bytes32 cType, uint256 ver) external view returns (bytes32); - function getContractTypeOwner(bytes32 cType) external view returns (address); + function getAllowedBytecodeHash(bytes32 contractType, uint256 version) external view returns (bytes32); + function getContractTypeOwner(bytes32 contractType) external view returns (address); function allowSystemContract(bytes32 bytecodeHash) external; function allowPublicContract(bytes32 bytecodeHash) external; - function removePublicContractType(bytes32 cType) external; + function removePublicContractType(bytes32 contractType) external; // ------------------ // // DOMAINS MANAGEMENT // @@ -152,8 +157,8 @@ interface IBytecodeRepository is IVersion, IImmutableOwnableTrait { // VERSION CONTROL // // --------------- // - function getVersions(bytes32 cType) external view returns (uint256[] memory); - function getLatestVersion(bytes32 cType) external view returns (uint256); - function getLatestMinorVersion(bytes32 cType, uint256 majorVersion) external view returns (uint256); - function getLatestPatchVersion(bytes32 cType, uint256 minorVersion) external view returns (uint256); + function getVersions(bytes32 contractType) external view returns (uint256[] memory); + function getLatestVersion(bytes32 contractType) external view returns (uint256); + function getLatestMinorVersion(bytes32 contractType, uint256 majorVersion) external view returns (uint256); + function getLatestPatchVersion(bytes32 contractType, uint256 minorVersion) external view returns (uint256); } diff --git a/contracts/market/MarketConfigurator.sol b/contracts/market/MarketConfigurator.sol index 3c01094..a887127 100644 --- a/contracts/market/MarketConfigurator.sol +++ b/contracts/market/MarketConfigurator.sol @@ -991,7 +991,7 @@ contract MarketConfigurator is DeployerTrait, IMarketConfigurator { } } - /// @dev Returns latest patch in the address provider for given contract type and minor version + /// @dev Returns latest patch in the address provider for given contract type with matching minor version function _getLatestPatch(bytes32 key, uint256 minorVersion) internal view returns (address) { return _getAddressOrRevert(key, IAddressProvider(addressProvider).getLatestPatchVersion(key, minorVersion)); } diff --git a/contracts/market/legacy/MarketConfiguratorLegacy.sol b/contracts/market/legacy/MarketConfiguratorLegacy.sol index 4e493c8..91cf628 100644 --- a/contracts/market/legacy/MarketConfiguratorLegacy.sol +++ b/contracts/market/legacy/MarketConfiguratorLegacy.sol @@ -206,11 +206,10 @@ contract MarketConfiguratorLegacy is MarketConfigurator { address rateKeeper = _rateKeeper(quotaKeeper); address lossPolicy = IContractsRegister(contractsRegister).getLossPolicy(pool); - // NOTE: authorize factories for contracts that might be used after the migration; - // legacy price oracle is left unauthorized since it's not gonna be used after the migration + // NOTE: authorize factories for contracts that might still be used after the migration; legacy price oracle + // is left unauthorized since it's not gonna be used, IRM is unauthorized since it's not configurable _authorizeFactory(factories.poolFactory, pool, pool); _authorizeFactory(factories.poolFactory, pool, quotaKeeper); - _authorizeFactory(factories.interestRateModelFactory, pool, interestRateModel); _authorizeFactory(factories.rateKeeperFactory, pool, rateKeeper); _authorizeFactory(factories.lossPolicyFactory, pool, lossPolicy); diff --git a/contracts/test/configuration/ConfigurationTestHelper.sol b/contracts/test/configuration/ConfigurationTestHelper.sol index fe4c41f..a2b68fb 100644 --- a/contracts/test/configuration/ConfigurationTestHelper.sol +++ b/contracts/test/configuration/ConfigurationTestHelper.sol @@ -146,9 +146,9 @@ contract ConfigurationTestHelper is Test, GlobalSetup { bytes32 bytecodeHash = _uploadByteCodeAndSign(type(MockLossPolicy).creationCode, "LOSS_POLICY::MOCK", 3_10); - calls[0] = _generateAllowSystemContractCall(bytecodeHash); + calls[0] = _generateAllowPublicContractCall(bytecodeHash); - _submitBatchAndSign("Allow system contracts", calls); + _submitBatchAndSign("Allow public contracts", calls); } function _deployTestPool() internal returns (address) { diff --git a/contracts/test/configuration/InterestRateModelConfiguration.unit.t.sol b/contracts/test/configuration/InterestRateModelConfiguration.unit.t.sol index b98a28c..1f49127 100644 --- a/contracts/test/configuration/InterestRateModelConfiguration.unit.t.sol +++ b/contracts/test/configuration/InterestRateModelConfiguration.unit.t.sol @@ -20,8 +20,8 @@ contract InterestRateModelConfigurationUnitTest is ConfigurationTestHelper { function test_IRM_01_configure() public { CrossChainCall[] memory calls = new CrossChainCall[](1); bytes32 bytecodeHash = _uploadByteCodeAndSign(type(MockIRM).creationCode, "IRM::MOCK", 3_10); - calls[0] = _generateAllowSystemContractCall(bytecodeHash); - _submitBatchAndSign("Allow system contracts", calls); + calls[0] = _generateAllowPublicContractCall(bytecodeHash); + _submitBatchAndSign("Allow public contracts", calls); vm.prank(admin); address newIRM = marketConfigurator.updateInterestRateModel( diff --git a/contracts/test/global/InstanceManager.unit.t.sol b/contracts/test/global/InstanceManager.unit.t.sol index 1e1a8b8..352450f 100644 --- a/contracts/test/global/InstanceManager.unit.t.sol +++ b/contracts/test/global/InstanceManager.unit.t.sol @@ -294,10 +294,9 @@ contract InstanceManagerTest is Test { "DEPLOYMENT_FAILED" ); + vm.expectRevert("DEPLOYMENT_FAILED"); vm.prank(crossChainGovernance); manager.deploySystemContract(contractType, version, false); - // Should not update address if deployment failed - assertEq(addressProvider.getAddressOrRevert(contractType, NO_VERSION_CONTROL), newContract); } /// @notice Test legacy GEAR staking deployment diff --git a/contracts/test/global/PriceFeedStore.unit.t.sol b/contracts/test/global/PriceFeedStore.unit.t.sol index 650be35..af00ac3 100644 --- a/contracts/test/global/PriceFeedStore.unit.t.sol +++ b/contracts/test/global/PriceFeedStore.unit.t.sol @@ -488,6 +488,7 @@ contract PriceFeedStoreTest is Test { abi.encodeWithSignature("isDeployedFromRepository(address)", address(ownableFeed)), abi.encode(true) ); + vm.mockCall(address(ownableFeed), abi.encodeWithSignature("acceptOwnership()"), ""); vm.mockCall(address(ownableFeed), abi.encodeWithSignature("owner()"), abi.encode(address(store))); // Test Ownable feed owned by other (should fail) @@ -497,20 +498,9 @@ contract PriceFeedStoreTest is Test { abi.encodeWithSignature("isDeployedFromRepository(address)", address(wrongOwnerFeed)), abi.encode(true) ); + vm.mockCall(address(wrongOwnerFeed), abi.encodeWithSignature("acceptOwnership()"), ""); vm.mockCall(address(wrongOwnerFeed), abi.encodeWithSignature("owner()"), abi.encode(makeAddr("other"))); - // Test Ownable2Step feed with pending transfer (should fail) - MockPriceFeed ownable2StepFeed = new MockPriceFeed(); - vm.mockCall( - address(bytecodeRepository), - abi.encodeWithSignature("isDeployedFromRepository(address)", address(ownable2StepFeed)), - abi.encode(true) - ); - vm.mockCall(address(ownable2StepFeed), abi.encodeWithSignature("owner()"), abi.encode(address(store))); - vm.mockCall( - address(ownable2StepFeed), abi.encodeWithSignature("pendingOwner()"), abi.encode(makeAddr("pending")) - ); - vm.startPrank(owner); // Non-ownable should pass @@ -525,12 +515,6 @@ contract PriceFeedStoreTest is Test { ); store.addPriceFeed(address(wrongOwnerFeed), 3600, "Wrong Owner Feed"); - // Pending transfer should fail - vm.expectRevert( - abi.encodeWithSelector(IPriceFeedStore.PriceFeedIsNotOwnedByStore.selector, address(ownable2StepFeed)) - ); - store.addPriceFeed(address(ownable2StepFeed), 3600, "Ownable2Step Feed"); - vm.stopPrank(); } @@ -574,36 +558,20 @@ contract PriceFeedStoreTest is Test { store.configurePriceFeeds(calls); } - function test_PFS_24_configurePriceFeeds_reverts_on_ownership_transfer() public { + function test_PFS_24_configurePriceFeeds_reverts_on_renounceOwnership() public { vm.prank(owner); store.addPriceFeed(address(priceFeed), 3600, "ETH/USD"); - Call[] memory transferCall = new Call[](1); - transferCall[0] = - Call(address(priceFeed), abi.encodeWithSignature("transferOwnership(address)", makeAddr("newOwner"))); - Call[] memory renounceCall = new Call[](1); renounceCall[0] = Call(address(priceFeed), abi.encodeWithSignature("renounceOwnership()")); - vm.startPrank(owner); - - // Test transferOwnership - vm.expectRevert( - abi.encodeWithSelector( - IPriceFeedStore.ForbiddenConfigurationMethodException.selector, bytes4(transferCall[0].callData) - ) - ); - store.configurePriceFeeds(transferCall); - - // Test renounceOwnership vm.expectRevert( abi.encodeWithSelector( IPriceFeedStore.ForbiddenConfigurationMethodException.selector, bytes4(renounceCall[0].callData) ) ); + vm.prank(owner); store.configurePriceFeeds(renounceCall); - - vm.stopPrank(); } function test_PFS_25_removePriceFeed_works() public { diff --git a/contracts/test/helpers/GlobalSetup.sol b/contracts/test/helpers/GlobalSetup.sol index a0ab2d2..37813fb 100644 --- a/contracts/test/helpers/GlobalSetup.sol +++ b/contracts/test/helpers/GlobalSetup.sol @@ -10,6 +10,7 @@ import {PriceFeedStore} from "../../instance/PriceFeedStore.sol"; import {IBytecodeRepository} from "../../interfaces/IBytecodeRepository.sol"; import {IAddressProvider} from "../../interfaces/IAddressProvider.sol"; import {IInstanceManager} from "../../interfaces/IInstanceManager.sol"; +import {Domain} from "../../libraries/Domain.sol"; import {IWETH} from "@gearbox-protocol/core-v3/contracts/interfaces/external/IWETH.sol"; import {IERC20} from "@openzeppelin/contracts/interfaces/IERC20.sol"; @@ -40,7 +41,15 @@ import { AP_LOSS_POLICY_DEFAULT, AP_CREDIT_MANAGER, AP_CREDIT_FACADE, - AP_CREDIT_CONFIGURATOR + AP_CREDIT_CONFIGURATOR, + DOMAIN_ADAPTER, + DOMAIN_BOT, + DOMAIN_DEGEN_NFT, + DOMAIN_IRM, + DOMAIN_LOSS_POLICY, + DOMAIN_PRICE_FEED, + DOMAIN_RATE_KEEPER, + DOMAIN_ZAPPER } from "../../libraries/ContractLiterals.sol"; import {SignedBatch, Bytecode} from "../../interfaces/Types.sol"; @@ -121,8 +130,6 @@ import {CurveCryptoLPPriceFeed} from "@gearbox-protocol/oracles-v3/contracts/ora import {CurveStableLPPriceFeed} from "@gearbox-protocol/oracles-v3/contracts/oracles/curve/CurveStableLPPriceFeed.sol"; import {ERC4626PriceFeed} from "@gearbox-protocol/oracles-v3/contracts/oracles/erc4626/ERC4626PriceFeed.sol"; -import {console} from "forge-std/console.sol"; - struct UploadableContract { bytes initCode; bytes32 contractType; @@ -142,7 +149,10 @@ contract GlobalSetup is Test, InstanceManagerHelper { constructor() { _setCoreContracts(); _setAdapters(); + _setInterestRateModels(); + _setLossPolicies(); _setPriceFeeds(); + _setRateKeepers(); } function _setUpGlobalContracts() internal { @@ -150,21 +160,40 @@ contract GlobalSetup is Test, InstanceManagerHelper { CrossChainCall[] memory calls = new CrossChainCall[](1); calls[0] = _generateAddAuditorCall(auditor, "Initial Auditor"); - - _submitBatchAndSign("Add Auditor", calls); + _submitBatchAndSign("Add auditor", calls); + + bytes32[8] memory publicDomains = [ + DOMAIN_ADAPTER, + DOMAIN_BOT, + DOMAIN_DEGEN_NFT, + DOMAIN_IRM, + DOMAIN_LOSS_POLICY, + DOMAIN_PRICE_FEED, + DOMAIN_RATE_KEEPER, + DOMAIN_ZAPPER + ]; + calls = new CrossChainCall[](publicDomains.length); + for (uint256 i = 0; i < publicDomains.length; ++i) { + calls[i] = _generateAddPublicDomainCall(publicDomains[i]); + } + _submitBatchAndSign("Add public domains", calls); uint256 len = contractsToUpload.length; - calls = new CrossChainCall[](len); - for (uint256 i = 0; i < len; ++i) { bytes32 bytecodeHash = _uploadByteCodeAndSign( contractsToUpload[i].initCode, contractsToUpload[i].contractType, contractsToUpload[i].version ); - calls[i] = _generateAllowSystemContractCall(bytecodeHash); - } - _submitBatchAndSign("Allow system contracts", calls); + bool isPublicContract = IBytecodeRepository(bytecodeRepository).isPublicDomain( + Domain.extractDomain(contractsToUpload[i].contractType) + ); + // NOTE: allowing public contracts doesn't require CCG permissions but it's convenient to execute in batch + calls[i] = isPublicContract + ? _generateAllowPublicContractCall(bytecodeHash) + : _generateAllowSystemContractCall(bytecodeHash); + } + _submitBatchAndSign("Allow contracts", calls); DeploySystemContractCall[10] memory deployCalls = [ DeploySystemContractCall({contractType: AP_BOT_LIST, version: 3_10, saveVersion: false}), @@ -178,17 +207,14 @@ contract GlobalSetup is Test, InstanceManagerHelper { DeploySystemContractCall({contractType: AP_RATE_KEEPER_FACTORY, version: 3_10, saveVersion: true}), DeploySystemContractCall({contractType: AP_LOSS_POLICY_FACTORY, version: 3_10, saveVersion: true}) ]; - len = deployCalls.length; - calls = new CrossChainCall[](len); for (uint256 i = 0; i < len; ++i) { calls[i] = _generateDeploySystemContractCall( deployCalls[i].contractType, deployCalls[i].version, deployCalls[i].saveVersion ); } - - _submitBatchAndSign("System contracts", calls); + _submitBatchAndSign("Deploy system contracts", calls); } function _attachGlobalContracts() internal { @@ -289,26 +315,6 @@ contract GlobalSetup is Test, InstanceManagerHelper { }) ); - contractsToUpload.push( - UploadableContract({ - initCode: type(LinearInterestRateModelV3).creationCode, - contractType: AP_INTEREST_RATE_MODEL_LINEAR, - version: 3_10 - }) - ); - - contractsToUpload.push( - UploadableContract({ - initCode: type(TumblerV3).creationCode, - contractType: AP_RATE_KEEPER_TUMBLER, - version: 3_10 - }) - ); - - contractsToUpload.push( - UploadableContract({initCode: type(GaugeV3).creationCode, contractType: AP_RATE_KEEPER_GAUGE, version: 3_10}) - ); - contractsToUpload.push( UploadableContract({initCode: type(BotListV3).creationCode, contractType: AP_BOT_LIST, version: 3_10}) ); @@ -337,14 +343,6 @@ contract GlobalSetup is Test, InstanceManagerHelper { }) ); - contractsToUpload.push( - UploadableContract({ - initCode: type(AliasedLossPolicyV3).creationCode, - contractType: AP_LOSS_POLICY_ALIASED, - version: 3_10 - }) - ); - contractsToUpload.push( UploadableContract({ initCode: type(MarketConfigurator).creationCode, @@ -399,7 +397,6 @@ contract GlobalSetup is Test, InstanceManagerHelper { } function _setAdapters() internal { - // TODO: set adapters contractsToUpload.push( UploadableContract({ initCode: type(EqualizerRouterAdapter).creationCode, @@ -593,15 +590,34 @@ contract GlobalSetup is Test, InstanceManagerHelper { ); } + function _setInterestRateModels() internal { + contractsToUpload.push( + UploadableContract({ + initCode: type(LinearInterestRateModelV3).creationCode, + contractType: "IRM::LINEAR", + version: 3_10 + }) + ); + } + + function _setLossPolicies() internal { + contractsToUpload.push( + UploadableContract({ + initCode: type(AliasedLossPolicyV3).creationCode, + contractType: "LOSS_POLICY::ALIASED", + version: 3_10 + }) + ); + } + function _setPriceFeeds() internal { - // TODO: set price feeds - // contractsToUpload.push( - // UploadableContract({ - // initCode: type(BPTWeightedPriceFeed).creationCode, - // contractType: "PRICE_FEED::BALANCER_WEIGHTED", - // version: 3_10 - // }) - // ); + contractsToUpload.push( + UploadableContract({ + initCode: type(BPTWeightedPriceFeed).creationCode, + contractType: "PRICE_FEED::BALANCER_WEIGHTED", + version: 3_10 + }) + ); contractsToUpload.push( UploadableContract({ @@ -716,6 +732,20 @@ contract GlobalSetup is Test, InstanceManagerHelper { ); } + function _setRateKeepers() internal { + contractsToUpload.push( + UploadableContract({initCode: type(GaugeV3).creationCode, contractType: "RATE_KEEPER::GAUGE", version: 3_10}) + ); + + contractsToUpload.push( + UploadableContract({ + initCode: type(TumblerV3).creationCode, + contractType: "RATE_KEEPER::TUMBLER", + version: 3_10 + }) + ); + } + // function _setupPriceFeedStore() internal { // // _addPriceFeed(CHAINLINK_ETH_USD, 1 days); // // _addPriceFeed(CHAINLINK_USDC_USD, 1 days); diff --git a/contracts/test/helpers/InstanceManagerHelper.sol b/contracts/test/helpers/InstanceManagerHelper.sol index 2796500..27b5950 100644 --- a/contracts/test/helpers/InstanceManagerHelper.sol +++ b/contracts/test/helpers/InstanceManagerHelper.sol @@ -91,6 +91,12 @@ contract InstanceManagerHelper is BCRHelpers, CCGHelper { ); } + function _generateAllowPublicContractCall(bytes32 _bytecodeHash) internal view returns (CrossChainCall memory) { + return _buildCrossChainCallDAO( + bytecodeRepository, abi.encodeCall(IBytecodeRepository.allowPublicContract, (_bytecodeHash)) + ); + } + function _generateDeploySystemContractCall(bytes32 _contractName, uint256 _version, bool _saveVersion) internal view @@ -103,6 +109,11 @@ contract InstanceManagerHelper is BCRHelpers, CCGHelper { }); } + function _generateAddPublicDomainCall(bytes32 domain) internal view returns (CrossChainCall memory) { + return + _buildCrossChainCallDAO(bytecodeRepository, abi.encodeCall(IBytecodeRepository.addPublicDomain, (domain))); + } + function _generateActivateCall( uint256 _chainId, address _instanceOwner, diff --git a/contracts/test/suite/NewChainDeploySuite.sol b/contracts/test/suite/NewChainDeploySuite.sol index 1bf55d1..3ca3e9e 100644 --- a/contracts/test/suite/NewChainDeploySuite.sol +++ b/contracts/test/suite/NewChainDeploySuite.sol @@ -109,9 +109,9 @@ contract NewChainDeploySuite is Test, GlobalSetup { bytes32 bytecodeHash = _uploadByteCodeAndSign(type(MockLossPolicy).creationCode, "LOSS_POLICY::MOCK", 3_10); - calls[0] = _generateAllowSystemContractCall(bytecodeHash); + calls[0] = _generateAllowPublicContractCall(bytecodeHash); - _submitBatchAndSign("Allow system contracts", calls); + _submitBatchAndSign("Allow public contracts", calls); } function _setupPriceFeedStore() internal { diff --git a/lib/@gearbox-protocol/core-v3 b/lib/@gearbox-protocol/core-v3 index f9894cb..a94cb84 160000 --- a/lib/@gearbox-protocol/core-v3 +++ b/lib/@gearbox-protocol/core-v3 @@ -1 +1 @@ -Subproject commit f9894cb5ce8c0f6fd3dd4b233e8d72f953d2fb05 +Subproject commit a94cb842d221594fff4fac92253d316c24bcad7e diff --git a/wagmi.config.js b/wagmi.config.js index 42f9edd..69b65e5 100644 --- a/wagmi.config.js +++ b/wagmi.config.js @@ -17,10 +17,6 @@ export default defineConfig({ "IPoolConfigureActions.sol/**.json", "IPriceOracleConfigureActions.sol/**.json", "ICreditConfigureActions.sol/**.json", - "ITumblerV3.sol/**.json", - "IGaugeV3.sol/**.json", - "IAliasedLossPolicyV3.sol/**.json", - "DefaultLossPolicy.sol/**.json", ], }), ],