From b718ddb2dc8f8d849f61cb9992ac7b565e03082d Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Mon, 11 Mar 2024 23:03:51 +0800 Subject: [PATCH 1/6] set subnet owner explicitly --- contracts/src/SubnetActorDiamond.sol | 4 ++-- contracts/src/subnetregistry/RegisterSubnetFacet.sol | 2 +- contracts/test/IntegrationTestBase.sol | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/src/SubnetActorDiamond.sol b/contracts/src/SubnetActorDiamond.sol index bde799f13..4aafeb0ba 100644 --- a/contracts/src/SubnetActorDiamond.sol +++ b/contracts/src/SubnetActorDiamond.sol @@ -38,7 +38,7 @@ contract SubnetActorDiamond { SubnetID parentId; } - constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params) { + constructor(IDiamond.FacetCut[] memory _diamondCut, ConstructorParams memory params, address owner) { if (params.ipcGatewayAddr == address(0)) { revert GatewayCannotBeZero(); } @@ -58,7 +58,7 @@ contract SubnetActorDiamond { params.supplySource.validate(); - LibDiamond.setContractOwner(msg.sender); + LibDiamond.setContractOwner(owner); LibDiamond.diamondCut({_diamondCut: _diamondCut, _init: address(0), _calldata: new bytes(0)}); LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage(); diff --git a/contracts/src/subnetregistry/RegisterSubnetFacet.sol b/contracts/src/subnetregistry/RegisterSubnetFacet.sol index c3480272c..9fcb31c59 100644 --- a/contracts/src/subnetregistry/RegisterSubnetFacet.sol +++ b/contracts/src/subnetregistry/RegisterSubnetFacet.sol @@ -58,7 +58,7 @@ contract RegisterSubnetFacet is ReentrancyGuard { }); // slither-disable-next-line reentrancy-benign - subnetAddr = address(new SubnetActorDiamond(diamondCut, _params)); + subnetAddr = address(new SubnetActorDiamond(diamondCut, _params, msg.sender)); //nonces start with 1, similar to eip 161 ++s.userNonces[msg.sender]; diff --git a/contracts/test/IntegrationTestBase.sol b/contracts/test/IntegrationTestBase.sol index e71c575d2..92178d708 100644 --- a/contracts/test/IntegrationTestBase.sol +++ b/contracts/test/IntegrationTestBase.sol @@ -407,7 +407,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); - saDiamond = new SubnetActorDiamond(diamondCut, params); + saDiamond = new SubnetActorDiamond(diamondCut, params, address(this)); return saDiamond; } @@ -478,7 +478,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); - SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params); + SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params, address(this)); return diamond; } @@ -555,7 +555,7 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, SubnetActorDiamond.ConstructorParams memory params = defaultSubnetActorParamsWith(gw); - SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params); + SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params, address(this)); return d; } From 3e2360db14a9d58232b4a58d575979ccc9fcb0de Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Tue, 12 Mar 2024 14:03:08 +0800 Subject: [PATCH 2/6] add transfer ownership --- contracts/src/GatewayDiamond.sol | 3 +- contracts/src/Ownable.sol | 14 ++++++++ contracts/src/SubnetActorDiamond.sol | 3 +- contracts/src/SubnetRegistryDiamond.sol | 4 ++- contracts/src/lib/LibDiamond.sol | 17 +++++++++ .../test/integration/GatewayDiamond.t.sol | 13 +++++++ contracts/test/unit/Ownable.t.sol | 35 +++++++++++++++++++ 7 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 contracts/src/Ownable.sol create mode 100644 contracts/test/unit/Ownable.t.sol diff --git a/contracts/src/GatewayDiamond.sol b/contracts/src/GatewayDiamond.sol index 9a87be704..f07424c8b 100644 --- a/contracts/src/GatewayDiamond.sol +++ b/contracts/src/GatewayDiamond.sol @@ -13,6 +13,7 @@ import {LibGateway} from "./lib/LibGateway.sol"; import {SubnetID} from "./structs/Subnet.sol"; import {LibStaking} from "./lib/LibStaking.sol"; import {BATCH_PERIOD, MAX_MSGS_PER_BATCH} from "./structs/CrossNet.sol"; +import {Ownable} from "./Ownable.sol"; error FunctionNotFound(bytes4 _functionSelector); @@ -20,7 +21,7 @@ bool constant FEATURE_MULTILEVEL_CROSSMSG = false; bool constant FEATURE_GENERAL_PUPRPOSE_CROSSMSG = true; uint8 constant FEATURE_SUBNET_DEPTH = 2; -contract GatewayDiamond { +contract GatewayDiamond is Ownable { GatewayActorStorage internal s; struct ConstructorParams { diff --git a/contracts/src/Ownable.sol b/contracts/src/Ownable.sol new file mode 100644 index 000000000..62869f531 --- /dev/null +++ b/contracts/src/Ownable.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity ^0.8.23; + +import {LibDiamond} from "./lib/LibDiamond.sol"; + +contract Ownable { + function owner() public view returns (address) { + return LibDiamond.contractOwner(); + } + + function transferOwnership(address newOwner) public { + LibDiamond.transferOwnership(newOwner); + } +} diff --git a/contracts/src/SubnetActorDiamond.sol b/contracts/src/SubnetActorDiamond.sol index 4aafeb0ba..c449a31ad 100644 --- a/contracts/src/SubnetActorDiamond.sol +++ b/contracts/src/SubnetActorDiamond.sol @@ -15,10 +15,11 @@ import {SubnetIDHelper} from "./lib/SubnetIDHelper.sol"; import {LibStaking} from "./lib/LibStaking.sol"; import {IERC20} from "openzeppelin-contracts/token/ERC20/IERC20.sol"; import {SupplySourceHelper} from "./lib/SupplySourceHelper.sol"; +import {Ownable} from "./Ownable.sol"; error FunctionNotFound(bytes4 _functionSelector); -contract SubnetActorDiamond { +contract SubnetActorDiamond is Ownable { SubnetActorStorage internal s; using SubnetIDHelper for SubnetID; diff --git a/contracts/src/SubnetRegistryDiamond.sol b/contracts/src/SubnetRegistryDiamond.sol index 244b5fcfb..e1596b273 100644 --- a/contracts/src/SubnetRegistryDiamond.sol +++ b/contracts/src/SubnetRegistryDiamond.sol @@ -8,9 +8,11 @@ import {IERC165} from "./interfaces/IERC165.sol"; import {SubnetRegistryActorStorage} from "./lib/LibSubnetRegistryStorage.sol"; import {GatewayCannotBeZero, FacetCannotBeZero} from "./errors/IPCErrors.sol"; import {LibDiamond} from "./lib/LibDiamond.sol"; +import {Ownable} from "./Ownable.sol"; + error FunctionNotFound(bytes4 _functionSelector); -contract SubnetRegistryDiamond { +contract SubnetRegistryDiamond is Ownable { SubnetRegistryActorStorage internal s; struct ConstructorParams { diff --git a/contracts/src/lib/LibDiamond.sol b/contracts/src/lib/LibDiamond.sol index 8cd100b61..c67dc9226 100644 --- a/contracts/src/lib/LibDiamond.sol +++ b/contracts/src/lib/LibDiamond.sol @@ -7,6 +7,7 @@ import {IDiamond} from "../interfaces/IDiamond.sol"; library LibDiamond { bytes32 public constant DIAMOND_STORAGE_POSITION = keccak256("libdiamond.lib.diamond.storage"); + error ZeroAddressNotAllowed(); error NotOwner(); error NoBytecodeAtAddress(address _contractAddress, string _message); error IncorrectFacetCutAction(IDiamondCut.FacetCutAction _action); @@ -24,6 +25,8 @@ library LibDiamond { error CannotRemoveFunctionThatDoesNotExist(bytes4 _selector); error CannotRemoveImmutableFunction(bytes4 _selector); + event OwnershipTransferred(address oldOwner, address newOwner); + struct FacetAddressAndSelectorPosition { address facetAddress; uint16 selectorPosition; @@ -38,6 +41,17 @@ library LibDiamond { address contractOwner; } + /** + * @dev Transfers ownership of the contract to a new account (`newOwner`). + * Can only be called by the current owner. + */ + function transferOwnership(address newOwner) internal onlyOwner { + if(newOwner == address(0)) { + revert ZeroAddressNotAllowed(); + } + setContractOwner(newOwner); + } + function diamondStorage() internal pure returns (DiamondStorage storage ds) { bytes32 position = DIAMOND_STORAGE_POSITION; assembly { @@ -47,7 +61,10 @@ library LibDiamond { function setContractOwner(address _newOwner) internal { DiamondStorage storage ds = diamondStorage(); + + address oldOwner = ds.contractOwner; ds.contractOwner = _newOwner; + emit OwnershipTransferred(oldOwner, _newOwner); } function contractOwner() internal view returns (address contractOwner_) { diff --git a/contracts/test/integration/GatewayDiamond.t.sol b/contracts/test/integration/GatewayDiamond.t.sol index 62562af54..581f4aa99 100644 --- a/contracts/test/integration/GatewayDiamond.t.sol +++ b/contracts/test/integration/GatewayDiamond.t.sol @@ -50,6 +50,19 @@ contract GatewayActorDiamondTest is Test, IntegrationTestBase { super.setUp(); } + function testGatewayDiamond_TransferOwnership() public { + address owner = gatewayDiamond.owner(); + + gatewayDiamond.transferOwnership(address(1)); + + address newOwner = gatewayDiamond.owner(); + require(owner != newOwner, "ownership should be updated"); + require(newOwner == address(1), "new owner not address 1"); + + vm.expectRevert(LibDiamond.NotOwner.selector); + gatewayDiamond.transferOwnership(address(1)); + } + function testGatewayDiamond_Constructor() public view { require(gatewayDiamond.getter().totalSubnets() == 0, "unexpected totalSubnets"); require(gatewayDiamond.getter().bottomUpNonce() == 0, "unexpected bottomUpNonce"); diff --git a/contracts/test/unit/Ownable.t.sol b/contracts/test/unit/Ownable.t.sol new file mode 100644 index 000000000..af6d6b230 --- /dev/null +++ b/contracts/test/unit/Ownable.t.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity ^0.8.23; + +import "forge-std/Test.sol"; + +import "fevmate/utils/FilAddress.sol"; +import {Ownable} from "../../src/Ownable.sol"; +import {LibDiamond} from "../../src/lib/LibDiamond.sol"; + +contract OwnableInstance is Ownable { + constructor(address owner) { + LibDiamond.setContractOwner(owner); + } +} + +contract OwnableTest is Test { + function testOwnableOk() public { + address firstOwner = address(1); + address secondOwner = address(2); + vm.deal(firstOwner, 1 ether); + vm.deal(secondOwner, 1 ether); + + OwnableInstance o = new OwnableInstance(firstOwner); + require(o.owner() == firstOwner, "owner not correct"); + + vm.prank(secondOwner); + vm.expectRevert(LibDiamond.NotOwner.selector); + o.transferOwnership(secondOwner); + + // owner can perform transfer + vm.prank(firstOwner); + o.transferOwnership(secondOwner); + require(o.owner() == secondOwner, "second owner not correct"); + } +} From c0264a05b805750100856cc6eb485efe79294b73 Mon Sep 17 00:00:00 2001 From: raulk Date: Wed, 13 Mar 2024 16:15:57 +0000 Subject: [PATCH 3/6] add extra test case; generalize error name. --- contracts/src/lib/LibDiamond.sol | 6 +++--- contracts/test/unit/Ownable.t.sol | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/contracts/src/lib/LibDiamond.sol b/contracts/src/lib/LibDiamond.sol index c67dc9226..e60f520c2 100644 --- a/contracts/src/lib/LibDiamond.sol +++ b/contracts/src/lib/LibDiamond.sol @@ -7,7 +7,7 @@ import {IDiamond} from "../interfaces/IDiamond.sol"; library LibDiamond { bytes32 public constant DIAMOND_STORAGE_POSITION = keccak256("libdiamond.lib.diamond.storage"); - error ZeroAddressNotAllowed(); + error InvalidAddress(); error NotOwner(); error NoBytecodeAtAddress(address _contractAddress, string _message); error IncorrectFacetCutAction(IDiamondCut.FacetCutAction _action); @@ -46,8 +46,8 @@ library LibDiamond { * Can only be called by the current owner. */ function transferOwnership(address newOwner) internal onlyOwner { - if(newOwner == address(0)) { - revert ZeroAddressNotAllowed(); + if (newOwner == address(0)) { + revert InvalidAddress(); } setContractOwner(newOwner); } diff --git a/contracts/test/unit/Ownable.t.sol b/contracts/test/unit/Ownable.t.sol index af6d6b230..64d70325b 100644 --- a/contracts/test/unit/Ownable.t.sol +++ b/contracts/test/unit/Ownable.t.sol @@ -14,7 +14,7 @@ contract OwnableInstance is Ownable { } contract OwnableTest is Test { - function testOwnableOk() public { + function testOwnable() public { address firstOwner = address(1); address secondOwner = address(2); vm.deal(firstOwner, 1 ether); @@ -31,5 +31,10 @@ contract OwnableTest is Test { vm.prank(firstOwner); o.transferOwnership(secondOwner); require(o.owner() == secondOwner, "second owner not correct"); + + // new owner cannot set zero address as next owner + vm.prank(secondOwner); + vm.expectRevert(LibDiamond.InvalidAddress.selector); + o.transferOwnership(address(0)); } } From bc031d86eff11efc564745648a07121b344aa752 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Thu, 14 Mar 2024 14:52:03 +0800 Subject: [PATCH 4/6] ownable facet --- contracts/scripts/deploy-gateway.template.ts | 1 + contracts/scripts/deploy-registry.template.ts | 1 + contracts/scripts/deploy-sa-diamond.ts | 1 + .../scripts/python/build_selector_library.py | 1 + contracts/src/GatewayDiamond.sol | 3 +- contracts/src/Ownable.sol | 14 ---- contracts/src/OwnershipFacet.sol | 14 ++++ contracts/src/SubnetActorDiamond.sol | 4 +- contracts/src/SubnetRegistryDiamond.sol | 3 +- contracts/test/IntegrationTestBase.sol | 67 +++++++++++++++++-- .../test/helpers/GatewayFacetsHelper.sol | 10 +++ contracts/test/helpers/SelectorLibrary.sol | 7 ++ .../test/integration/GatewayDiamond.t.sol | 13 ++-- .../test/integration/SubnetActorDiamond.t.sol | 7 +- .../test/integration/SubnetRegistry.t.sol | 2 +- contracts/test/unit/Ownable.t.sol | 35 ---------- 16 files changed, 113 insertions(+), 70 deletions(-) delete mode 100644 contracts/src/Ownable.sol create mode 100644 contracts/src/OwnershipFacet.sol delete mode 100644 contracts/test/unit/Ownable.t.sol diff --git a/contracts/scripts/deploy-gateway.template.ts b/contracts/scripts/deploy-gateway.template.ts index 5200c668c..035aa21d9 100644 --- a/contracts/scripts/deploy-gateway.template.ts +++ b/contracts/scripts/deploy-gateway.template.ts @@ -92,6 +92,7 @@ export async function deploy(libs: { [key in string]: string }) { libs: xnetMessagingFacetLibs, }, { name: 'TopDownFinalityFacet', libs: topDownFinalityFacetLibs }, + { name: 'OwnershipFacet', libs: {} }, ] for (const facet of facets) { diff --git a/contracts/scripts/deploy-registry.template.ts b/contracts/scripts/deploy-registry.template.ts index 80cac756d..f77b1a3a8 100644 --- a/contracts/scripts/deploy-registry.template.ts +++ b/contracts/scripts/deploy-registry.template.ts @@ -90,6 +90,7 @@ export async function deploy() { { name: 'SubnetGetterFacet', libs: {} }, { name: 'DiamondLoupeFacet', libs: {} }, { name: 'DiamondCutFacet', libs: {} }, + { name: 'OwnershipFacet', libs: {} }, ] for (const facet of facets) { diff --git a/contracts/scripts/deploy-sa-diamond.ts b/contracts/scripts/deploy-sa-diamond.ts index 0cc5fa439..60dc56a90 100644 --- a/contracts/scripts/deploy-sa-diamond.ts +++ b/contracts/scripts/deploy-sa-diamond.ts @@ -42,6 +42,7 @@ async function deploySubnetActorDiamond( { name: 'SubnetActorRewardFacet', libs: rewarderFacetLibs }, { name: 'SubnetActorCheckpointingFacet', libs: checkpointerFacetLibs }, { name: 'SubnetActorPauseFacet', libs: pauserFacetLibs }, + { name: 'OwnershipFacet', libs: {} }, ] // The `facetCuts` variable is the FacetCut[] that contains the functions to add during diamond deployment const facetCuts = [] diff --git a/contracts/scripts/python/build_selector_library.py b/contracts/scripts/python/build_selector_library.py index ee8b33fb8..2b0ebd4d9 100644 --- a/contracts/scripts/python/build_selector_library.py +++ b/contracts/scripts/python/build_selector_library.py @@ -70,6 +70,7 @@ def main(): 'src/GatewayDiamond.sol', 'src/SubnetActorDiamond.sol', 'src/SubnetRegistryDiamond.sol', + 'src/OwnershipFacet.sol', 'src/diamond/DiamondCutFacet.sol', 'src/diamond/DiamondLoupeFacet.sol', 'src/gateway/GatewayGetterFacet.sol', diff --git a/contracts/src/GatewayDiamond.sol b/contracts/src/GatewayDiamond.sol index f07424c8b..9a87be704 100644 --- a/contracts/src/GatewayDiamond.sol +++ b/contracts/src/GatewayDiamond.sol @@ -13,7 +13,6 @@ import {LibGateway} from "./lib/LibGateway.sol"; import {SubnetID} from "./structs/Subnet.sol"; import {LibStaking} from "./lib/LibStaking.sol"; import {BATCH_PERIOD, MAX_MSGS_PER_BATCH} from "./structs/CrossNet.sol"; -import {Ownable} from "./Ownable.sol"; error FunctionNotFound(bytes4 _functionSelector); @@ -21,7 +20,7 @@ bool constant FEATURE_MULTILEVEL_CROSSMSG = false; bool constant FEATURE_GENERAL_PUPRPOSE_CROSSMSG = true; uint8 constant FEATURE_SUBNET_DEPTH = 2; -contract GatewayDiamond is Ownable { +contract GatewayDiamond { GatewayActorStorage internal s; struct ConstructorParams { diff --git a/contracts/src/Ownable.sol b/contracts/src/Ownable.sol deleted file mode 100644 index 62869f531..000000000 --- a/contracts/src/Ownable.sol +++ /dev/null @@ -1,14 +0,0 @@ -// SPDX-License-Identifier: MIT OR Apache-2.0 -pragma solidity ^0.8.23; - -import {LibDiamond} from "./lib/LibDiamond.sol"; - -contract Ownable { - function owner() public view returns (address) { - return LibDiamond.contractOwner(); - } - - function transferOwnership(address newOwner) public { - LibDiamond.transferOwnership(newOwner); - } -} diff --git a/contracts/src/OwnershipFacet.sol b/contracts/src/OwnershipFacet.sol new file mode 100644 index 000000000..e321661a9 --- /dev/null +++ b/contracts/src/OwnershipFacet.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.23; + +import {LibDiamond} from "./lib/LibDiamond.sol"; + +contract OwnershipFacet { + function transferOwnership(address _newOwner) external { + LibDiamond.transferOwnership(_newOwner); + } + + function owner() external view returns (address owner_) { + owner_ = LibDiamond.contractOwner(); + } +} diff --git a/contracts/src/SubnetActorDiamond.sol b/contracts/src/SubnetActorDiamond.sol index c449a31ad..178e75d74 100644 --- a/contracts/src/SubnetActorDiamond.sol +++ b/contracts/src/SubnetActorDiamond.sol @@ -15,11 +15,9 @@ import {SubnetIDHelper} from "./lib/SubnetIDHelper.sol"; import {LibStaking} from "./lib/LibStaking.sol"; import {IERC20} from "openzeppelin-contracts/token/ERC20/IERC20.sol"; import {SupplySourceHelper} from "./lib/SupplySourceHelper.sol"; -import {Ownable} from "./Ownable.sol"; - error FunctionNotFound(bytes4 _functionSelector); -contract SubnetActorDiamond is Ownable { +contract SubnetActorDiamond { SubnetActorStorage internal s; using SubnetIDHelper for SubnetID; diff --git a/contracts/src/SubnetRegistryDiamond.sol b/contracts/src/SubnetRegistryDiamond.sol index e1596b273..5b7337e15 100644 --- a/contracts/src/SubnetRegistryDiamond.sol +++ b/contracts/src/SubnetRegistryDiamond.sol @@ -8,11 +8,10 @@ import {IERC165} from "./interfaces/IERC165.sol"; import {SubnetRegistryActorStorage} from "./lib/LibSubnetRegistryStorage.sol"; import {GatewayCannotBeZero, FacetCannotBeZero} from "./errors/IPCErrors.sol"; import {LibDiamond} from "./lib/LibDiamond.sol"; -import {Ownable} from "./Ownable.sol"; error FunctionNotFound(bytes4 _functionSelector); -contract SubnetRegistryDiamond is Ownable { +contract SubnetRegistryDiamond { SubnetRegistryActorStorage internal s; struct ConstructorParams { diff --git a/contracts/test/IntegrationTestBase.sol b/contracts/test/IntegrationTestBase.sol index 92178d708..9e72a0db0 100644 --- a/contracts/test/IntegrationTestBase.sol +++ b/contracts/test/IntegrationTestBase.sol @@ -35,6 +35,8 @@ import {SubnetRegistryDiamond} from "../src/SubnetRegistryDiamond.sol"; import {RegisterSubnetFacet} from "../src/subnetregistry/RegisterSubnetFacet.sol"; import {SubnetGetterFacet} from "../src/subnetregistry/SubnetGetterFacet.sol"; +import {OwnershipFacet} from "../src/OwnershipFacet.sol"; + import {DiamondLoupeFacet} from "../src/diamond/DiamondLoupeFacet.sol"; import {DiamondCutFacet} from "../src/diamond/DiamondCutFacet.sol"; import {SupplySourceHelper} from "../src/lib/SupplySourceHelper.sol"; @@ -89,18 +91,21 @@ contract TestRegistry is Test, TestParams { bytes4[] registerSubnetGetterFacetSelectors; bytes4[] registerCutterSelectors; bytes4[] registerLouperSelectors; + bytes4[] registerOwnershipSelectors; SubnetRegistryDiamond registryDiamond; DiamondLoupeFacet registryLouper; DiamondCutFacet registryCutter; RegisterSubnetFacet registrySubnetFacet; SubnetGetterFacet registrySubnetGetterFacet; + OwnershipFacet ownershipFacet; constructor() { registerSubnetFacetSelectors = SelectorLibrary.resolveSelectors("RegisterSubnetFacet"); registerSubnetGetterFacetSelectors = SelectorLibrary.resolveSelectors("SubnetGetterFacet"); registerCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet"); registerLouperSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet"); + registerOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet"); } } @@ -116,6 +121,8 @@ contract TestGatewayActor is Test, TestParams { bytes4[] gwCutterSelectors; bytes4[] gwLoupeSelectors; + bytes4[] gwOwnershipSelectors; + GatewayDiamond gatewayDiamond; constructor() { @@ -128,6 +135,8 @@ contract TestGatewayActor is Test, TestParams { gwMessengerSelectors = SelectorLibrary.resolveSelectors("GatewayMessengerFacet"); gwCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet"); gwLoupeSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet"); + + gwOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet"); } } @@ -140,6 +149,7 @@ contract TestSubnetActor is Test, TestParams { bytes4[] saManagerMockedSelectors; bytes4[] saCutterSelectors; bytes4[] saLouperSelectors; + bytes4[] saOwnershipSelectors; SubnetActorDiamond saDiamond; SubnetActorMock saMock; @@ -153,6 +163,7 @@ contract TestSubnetActor is Test, TestParams { saManagerMockedSelectors = SelectorLibrary.resolveSelectors("SubnetActorMock"); saCutterSelectors = SelectorLibrary.resolveSelectors("DiamondCutFacet"); saLouperSelectors = SelectorLibrary.resolveSelectors("DiamondLoupeFacet"); + saOwnershipSelectors = SelectorLibrary.resolveSelectors("OwnershipFacet"); } function defaultSubnetActorParamsWith( @@ -285,8 +296,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, GatewayMessengerFacet messenger = new GatewayMessengerFacet(); DiamondCutFacet cutter = new DiamondCutFacet(); DiamondLoupeFacet louper = new DiamondLoupeFacet(); + OwnershipFacet ownership = new OwnershipFacet(); - IDiamond.FacetCut[] memory gwDiamondCut = new IDiamond.FacetCut[](8); + IDiamond.FacetCut[] memory gwDiamondCut = new IDiamond.FacetCut[](9); gwDiamondCut[0] = ( IDiamond.FacetCut({ @@ -352,6 +364,13 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); + gwDiamondCut[8] = ( + IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: gwOwnershipSelectors + }) + ); gatewayDiamond = new GatewayDiamond(gwDiamondCut, params); return gatewayDiamond; @@ -363,9 +382,10 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, address manager, address pauser, address rewarder, - address checkpointer + address checkpointer, + address ownership ) public returns (SubnetActorDiamond) { - IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](5); + IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](6); diamondCut[0] = ( IDiamond.FacetCut({ @@ -407,6 +427,14 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); + diamondCut[5] = ( + IDiamond.FacetCut({ + facetAddress: ownership, + action: IDiamond.FacetCutAction.Add, + functionSelectors: saOwnershipSelectors + }) + ); + saDiamond = new SubnetActorDiamond(diamondCut, params, address(this)); return saDiamond; } @@ -419,8 +447,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, SubnetActorCheckpointingFacet checkpointer = new SubnetActorCheckpointingFacet(); DiamondLoupeFacet louper = new DiamondLoupeFacet(); DiamondCutFacet cutter = new DiamondCutFacet(); + OwnershipFacet ownership = new OwnershipFacet(); - IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](7); + IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](8); diamondCut[0] = ( IDiamond.FacetCut({ @@ -478,6 +507,14 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); + diamondCut[7] = ( + IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: saOwnershipSelectors + }) + ); + SubnetActorDiamond diamond = new SubnetActorDiamond(diamondCut, params, address(this)); return diamond; @@ -534,8 +571,9 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, function createMockedSubnetActorWithGateway(address gw) public returns (SubnetActorDiamond) { SubnetActorMock mockedManager = new SubnetActorMock(); SubnetActorGetterFacet getter = new SubnetActorGetterFacet(); + OwnershipFacet ownership = new OwnershipFacet(); - IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](2); + IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](3); diamondCut[0] = ( IDiamond.FacetCut({ @@ -553,6 +591,14 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); + diamondCut[2] = ( + IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: saOwnershipSelectors + }) + ); + SubnetActorDiamond.ConstructorParams memory params = defaultSubnetActorParamsWith(gw); SubnetActorDiamond d = new SubnetActorDiamond(diamondCut, params, address(this)); @@ -564,12 +610,13 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, function createSubnetRegistry( SubnetRegistryDiamond.ConstructorParams memory params ) public returns (SubnetRegistryDiamond) { - IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](4); + IDiamond.FacetCut[] memory diamondCut = new IDiamond.FacetCut[](5); DiamondCutFacet regCutFacet = new DiamondCutFacet(); DiamondLoupeFacet regLoupeFacet = new DiamondLoupeFacet(); RegisterSubnetFacet regSubnetFacet = new RegisterSubnetFacet(); SubnetGetterFacet regGetterFacet = new SubnetGetterFacet(); + OwnershipFacet ownership = new OwnershipFacet(); diamondCut[0] = ( IDiamond.FacetCut({ @@ -600,6 +647,14 @@ contract IntegrationTestBase is Test, TestParams, TestRegistry, TestSubnetActor, }) ); + diamondCut[4] = ( + IDiamond.FacetCut({ + facetAddress: address(ownership), + action: IDiamond.FacetCutAction.Add, + functionSelectors: registerOwnershipSelectors + }) + ); + SubnetRegistryDiamond newSubnetRegistry = new SubnetRegistryDiamond(diamondCut, params); emit SubnetRegistryCreated(address(newSubnetRegistry)); return newSubnetRegistry; diff --git a/contracts/test/helpers/GatewayFacetsHelper.sol b/contracts/test/helpers/GatewayFacetsHelper.sol index dfb4efd83..2aed980f9 100644 --- a/contracts/test/helpers/GatewayFacetsHelper.sol +++ b/contracts/test/helpers/GatewayFacetsHelper.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 pragma solidity ^0.8.23; +import {OwnershipFacet} from "../../src/OwnershipFacet.sol"; import {GatewayGetterFacet} from "../../src/gateway/GatewayGetterFacet.sol"; import {GatewayManagerFacet} from "../../src/gateway/GatewayManagerFacet.sol"; import {GatewayMessengerFacet} from "../../src/gateway/GatewayMessengerFacet.sol"; @@ -12,6 +13,11 @@ import {DiamondLoupeFacet} from "../../src/diamond/DiamondLoupeFacet.sol"; import {DiamondCutFacet} from "../../src/diamond/DiamondCutFacet.sol"; library GatewayFacetsHelper { + function ownership(address gw) internal pure returns (OwnershipFacet) { + OwnershipFacet facet = OwnershipFacet(gw); + return facet; + } + function getter(address gw) internal pure returns (GatewayGetterFacet) { GatewayGetterFacet facet = GatewayGetterFacet(gw); return facet; @@ -43,6 +49,10 @@ library GatewayFacetsHelper { } // + function ownership(GatewayDiamond gw) internal pure returns (OwnershipFacet) { + OwnershipFacet facet = OwnershipFacet(address(gw)); + return facet; + } function getter(GatewayDiamond gw) internal pure returns (GatewayGetterFacet) { GatewayGetterFacet facet = GatewayGetterFacet(address(gw)); diff --git a/contracts/test/helpers/SelectorLibrary.sol b/contracts/test/helpers/SelectorLibrary.sol index 1cfe63591..b068ae5c3 100644 --- a/contracts/test/helpers/SelectorLibrary.sol +++ b/contracts/test/helpers/SelectorLibrary.sol @@ -24,6 +24,13 @@ library SelectorLibrary { (bytes4[]) ); } + if (keccak256(abi.encodePacked(facetName)) == keccak256(abi.encodePacked("OwnershipFacet"))) { + return + abi.decode( + hex"000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000028da5cb5b00000000000000000000000000000000000000000000000000000000f2fde38b00000000000000000000000000000000000000000000000000000000", + (bytes4[]) + ); + } if (keccak256(abi.encodePacked(facetName)) == keccak256(abi.encodePacked("DiamondCutFacet"))) { return abi.decode( diff --git a/contracts/test/integration/GatewayDiamond.t.sol b/contracts/test/integration/GatewayDiamond.t.sol index 581f4aa99..7359ac949 100644 --- a/contracts/test/integration/GatewayDiamond.t.sol +++ b/contracts/test/integration/GatewayDiamond.t.sol @@ -51,16 +51,19 @@ contract GatewayActorDiamondTest is Test, IntegrationTestBase { } function testGatewayDiamond_TransferOwnership() public { - address owner = gatewayDiamond.owner(); + address owner = gatewayDiamond.ownership().owner(); - gatewayDiamond.transferOwnership(address(1)); + vm.expectRevert(LibDiamond.ZeroAddressNotAllowed.selector); + gatewayDiamond.ownership().transferOwnership(address(0)); - address newOwner = gatewayDiamond.owner(); + gatewayDiamond.ownership().transferOwnership(address(1)); + + address newOwner = gatewayDiamond.ownership().owner(); require(owner != newOwner, "ownership should be updated"); require(newOwner == address(1), "new owner not address 1"); vm.expectRevert(LibDiamond.NotOwner.selector); - gatewayDiamond.transferOwnership(address(1)); + gatewayDiamond.ownership().transferOwnership(address(1)); } function testGatewayDiamond_Constructor() public view { @@ -98,7 +101,7 @@ contract GatewayActorDiamondTest is Test, IntegrationTestBase { } function testGatewayDiamond_LoupeFunction() public view { - require(gatewayDiamond.diamondLouper().facets().length == 8, "unexpected length"); + require(gatewayDiamond.diamondLouper().facets().length == 9, "unexpected length"); require( gatewayDiamond.diamondLouper().supportsInterface(type(IERC165).interfaceId) == true, "IERC165 not supported" diff --git a/contracts/test/integration/SubnetActorDiamond.t.sol b/contracts/test/integration/SubnetActorDiamond.t.sol index e74f2e5b4..17b3f0d2a 100644 --- a/contracts/test/integration/SubnetActorDiamond.t.sol +++ b/contracts/test/integration/SubnetActorDiamond.t.sol @@ -24,6 +24,7 @@ import {SubnetIDHelper} from "../../src/lib/SubnetIDHelper.sol"; import {GatewayDiamond} from "../../src/GatewayDiamond.sol"; import {SubnetActorDiamond, FunctionNotFound} from "../../src/SubnetActorDiamond.sol"; import {SubnetActorManagerFacet} from "../../src/subnet/SubnetActorManagerFacet.sol"; +import {OwnershipFacet} from "../../src/OwnershipFacet.sol"; import {SubnetActorGetterFacet} from "../../src/subnet/SubnetActorGetterFacet.sol"; import {SubnetActorPauseFacet} from "../../src/subnet/SubnetActorPauseFacet.sol"; import {SubnetActorCheckpointingFacet} from "../../src/subnet/SubnetActorCheckpointingFacet.sol"; @@ -75,7 +76,7 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { } function testSubnetActorDiamondReal_LoupeFunction() public view { - require(saDiamond.diamondLouper().facets().length == 7, "unexpected length"); + require(saDiamond.diamondLouper().facets().length == 8, "unexpected length"); require( saDiamond.diamondLouper().supportsInterface(type(IERC165).interfaceId) == true, "IERC165 not supported" @@ -318,6 +319,7 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { SubnetActorPauseFacet saDupPauserFaucet = new SubnetActorPauseFacet(); SubnetActorRewardFacet saDupRewardFaucet = new SubnetActorRewardFacet(); SubnetActorCheckpointingFacet saDupCheckpointerFaucet = new SubnetActorCheckpointingFacet(); + OwnershipFacet saOwnershipFacet = new OwnershipFacet(); SupplySource memory native = SupplySourceHelper.native(); @@ -340,7 +342,8 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { address(saDupMangerFaucet), address(saDupPauserFaucet), address(saDupRewardFaucet), - address(saDupCheckpointerFaucet) + address(saDupCheckpointerFaucet), + address(saOwnershipFacet) ); } diff --git a/contracts/test/integration/SubnetRegistry.t.sol b/contracts/test/integration/SubnetRegistry.t.sol index 56076e56d..70ccb2b1f 100644 --- a/contracts/test/integration/SubnetRegistry.t.sol +++ b/contracts/test/integration/SubnetRegistry.t.sol @@ -88,7 +88,7 @@ contract SubnetRegistryTest is Test, TestRegistry, IntegrationTestBase { } function test_Registry_Deployment_IERC165() public view { - require(registryLouper.facets().length == 4, "unexpected length"); + require(registryLouper.facets().length == 5, "unexpected length"); require(registryLouper.facetAddresses().length == registryLouper.facets().length, "inconsistent diamond size"); require(registryLouper.supportsInterface(type(IERC165).interfaceId) == true, "IERC165 not supported"); require(registryLouper.supportsInterface(type(IDiamondCut).interfaceId) == true, "IDiamondCut not supported"); diff --git a/contracts/test/unit/Ownable.t.sol b/contracts/test/unit/Ownable.t.sol deleted file mode 100644 index af6d6b230..000000000 --- a/contracts/test/unit/Ownable.t.sol +++ /dev/null @@ -1,35 +0,0 @@ -// SPDX-License-Identifier: MIT OR Apache-2.0 -pragma solidity ^0.8.23; - -import "forge-std/Test.sol"; - -import "fevmate/utils/FilAddress.sol"; -import {Ownable} from "../../src/Ownable.sol"; -import {LibDiamond} from "../../src/lib/LibDiamond.sol"; - -contract OwnableInstance is Ownable { - constructor(address owner) { - LibDiamond.setContractOwner(owner); - } -} - -contract OwnableTest is Test { - function testOwnableOk() public { - address firstOwner = address(1); - address secondOwner = address(2); - vm.deal(firstOwner, 1 ether); - vm.deal(secondOwner, 1 ether); - - OwnableInstance o = new OwnableInstance(firstOwner); - require(o.owner() == firstOwner, "owner not correct"); - - vm.prank(secondOwner); - vm.expectRevert(LibDiamond.NotOwner.selector); - o.transferOwnership(secondOwner); - - // owner can perform transfer - vm.prank(firstOwner); - o.transferOwnership(secondOwner); - require(o.owner() == secondOwner, "second owner not correct"); - } -} From 7b9189dd0838183477bf88d5559e90f9aaef8d6b Mon Sep 17 00:00:00 2001 From: raulk Date: Thu, 14 Mar 2024 10:49:52 +0000 Subject: [PATCH 5/6] fix test after merge. --- contracts/test/integration/GatewayDiamond.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/test/integration/GatewayDiamond.t.sol b/contracts/test/integration/GatewayDiamond.t.sol index 7359ac949..2ecdd66cf 100644 --- a/contracts/test/integration/GatewayDiamond.t.sol +++ b/contracts/test/integration/GatewayDiamond.t.sol @@ -53,7 +53,7 @@ contract GatewayActorDiamondTest is Test, IntegrationTestBase { function testGatewayDiamond_TransferOwnership() public { address owner = gatewayDiamond.ownership().owner(); - vm.expectRevert(LibDiamond.ZeroAddressNotAllowed.selector); + vm.expectRevert(LibDiamond.InvalidAddress.selector); gatewayDiamond.ownership().transferOwnership(address(0)); gatewayDiamond.ownership().transferOwnership(address(1)); From f2b97c084de0ae5d5b6aeade270e752159e73c24 Mon Sep 17 00:00:00 2001 From: raulk Date: Thu, 14 Mar 2024 11:32:12 +0000 Subject: [PATCH 6/6] aderyn CI check: add ability to suppress specific findings. (#794) --- .github/workflows/contracts-sast.yaml | 2 +- contracts/.gitignore | 1 + contracts/audit-resolve.json | 4 ++ contracts/tools/check_aderyn.sh | 60 ++++++++++++++++++--------- 4 files changed, 46 insertions(+), 21 deletions(-) diff --git a/.github/workflows/contracts-sast.yaml b/.github/workflows/contracts-sast.yaml index fa2791e2d..04067a6d3 100644 --- a/.github/workflows/contracts-sast.yaml +++ b/.github/workflows/contracts-sast.yaml @@ -42,7 +42,7 @@ jobs: run: cargo install aderyn - name: Run aderyn - run: cd contracts && aderyn ./ + run: cd contracts && aderyn ./ -o report.json - name: Check results run: cd contracts && ./tools/check_aderyn.sh diff --git a/contracts/.gitignore b/contracts/.gitignore index e822d26d4..84ef71d4e 100644 --- a/contracts/.gitignore +++ b/contracts/.gitignore @@ -31,6 +31,7 @@ lcov.info # Aderyn scanner report.md +report.json #vim *.un~ diff --git a/contracts/audit-resolve.json b/contracts/audit-resolve.json index faa5cf2af..e85dcdfab 100644 --- a/contracts/audit-resolve.json +++ b/contracts/audit-resolve.json @@ -55,6 +55,10 @@ "1096544|json5": { "decision": "ignore", "madeAt": 1708963077846 + }, + "1096644|browserify-sign": { + "decision": "ignore", + "madeAt": 1710415772020 } }, "rules": {}, diff --git a/contracts/tools/check_aderyn.sh b/contracts/tools/check_aderyn.sh index 5c0b50ad5..debec6025 100755 --- a/contracts/tools/check_aderyn.sh +++ b/contracts/tools/check_aderyn.sh @@ -2,23 +2,43 @@ set -eu set -o pipefail -REPORT_FILE="./report.md" - -if [ ! -f $REPORT_FILE ]; then - echo "Report file not found." - exit 1; -fi - -# Check if one of `| Critical | 0 |`, `| High | 0 |`, or `| Medium | 0 |` line exist in the report. -zero_findings=`(grep -e "Critical\s*|\s*0" $REPORT_FILE && grep -e "High\s*|\s*0" $REPORT_FILE && grep -e "Medium\s*|\s*0" $REPORT_FILE) | wc -l` - -if [ $zero_findings -eq 3 ]; then - echo "No critical or high issues found" - exit 0 -else - echo "Critical, high, or medium issue found". - echo "Check $REPORT_FILE for more information". - echo "Printing here..." - cat $REPORT_FILE - exit 1; -fi +# Path to the report file +REPORT_FILE="./report.json" + +# List of severities that make us fail +SEVERITIES=(critical high medium) + +# List of vulnerability titles to ignore +IGNORE_TITLES=("Centralization Risk for trusted owners") + +containsElement() { + local e match="$1" + shift + for e; do [[ "$e" == "$match" ]] && return 0; done + return 1 +} + +# Read vulnerabilities from the report +readVulnerabilities() { + level="$1" + jq -c --argjson ignoreTitles "$(printf '%s\n' "${IGNORE_TITLES[@]}" | jq -R . | jq -s .)" ".${level}_issues.issues[] | select(.title as \$title | \$ignoreTitles | index(\$title) | not)" $REPORT_FILE +} + +# Main function to process the report +processReport() { + local hasVulnerabilities=0 + + for level in ${SEVERITIES[@]}; do + while IFS= read -r vulnerability; do + title=$(echo "$vulnerability" | jq -r ".title") + echo "Found $level vulnerability: $title" + hasVulnerabilities=1 + done < <(readVulnerabilities "$level") + done + + return $hasVulnerabilities +} + +# Process the report and exit with the code returned by processReport +processReport +exit $? \ No newline at end of file