From 4b9ae397eb08c9ec882bfcd2db6edf3e916e3802 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Mon, 20 Jan 2025 16:03:36 +0800 Subject: [PATCH 1/4] update contract audit --- .../contracts/gateway/GatewayManagerFacet.sol | 21 +++++++++++-------- .../gateway/GatewayMessengerFacet.sol | 2 +- contracts/contracts/lib/LibGateway.sol | 4 ++-- contracts/contracts/lib/LibSubnetActor.sol | 2 +- .../subnet/SubnetActorManagerFacet.sol | 14 +++++++++++-- .../test/integration/SubnetActorDiamond.t.sol | 2 +- 6 files changed, 29 insertions(+), 16 deletions(-) diff --git a/contracts/contracts/gateway/GatewayManagerFacet.sol b/contracts/contracts/gateway/GatewayManagerFacet.sol index a75480825..0e9c0f4f3 100644 --- a/contracts/contracts/gateway/GatewayManagerFacet.sol +++ b/contracts/contracts/gateway/GatewayManagerFacet.sol @@ -27,6 +27,8 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { using AssetHelper for Asset; using EnumerableSet for EnumerableSet.Bytes32Set; + event SubnetKilled(SubnetID id); + /// @notice register a subnet in the gateway. It is called by a subnet when it reaches the threshold stake /// @dev The subnet can optionally pass a genesis circulating supply that would be pre-allocated in the /// subnet from genesis (without having to wait for the subnet to be spawned to propagate the funds). @@ -37,13 +39,6 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { revert MethodNotAllowed(ERR_CHILD_SUBNET_NOT_ALLOWED); } - if (genesisCircSupply > 0) { - SubnetActorGetterFacet(msg.sender).supplySource().lock(genesisCircSupply); - } - if (collateral > 0) { - SubnetActorGetterFacet(msg.sender).collateralSource().lock(collateral); - } - SubnetID memory subnetId = s.networkName.createSubnetId(msg.sender); (bool registered, Subnet storage subnet) = LibGateway.getSubnet(subnetId); @@ -58,10 +53,17 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { s.subnetKeys.add(subnetId.toHash()); s.totalSubnets += 1; + + if (genesisCircSupply > 0) { + SubnetActorGetterFacet(msg.sender).supplySource().lock(genesisCircSupply); + } + if (collateral > 0) { + SubnetActorGetterFacet(msg.sender).collateralSource().lock(collateral); + } } /// @notice addStake - add collateral for an existing subnet - function addStake(uint256 amount) external payable { + function addStake(uint256 amount) external nonReentrant payable { if (amount == 0) { revert NotEnoughFunds(); } @@ -72,7 +74,6 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { SubnetActorGetterFacet(msg.sender).collateralSource().lock(amount); (bool registered, Subnet storage subnet) = LibGateway.getSubnet(msg.sender); - if (!registered) { revert NotRegisteredSubnet(); } @@ -126,6 +127,8 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { s.subnetKeys.remove(id); SubnetActorGetterFacet(msg.sender).collateralSource().transferFunds(payable(msg.sender), stake); + + emit SubnetKilled(subnet.id); } /// @notice credits the received value to the specified address in the specified child subnet. diff --git a/contracts/contracts/gateway/GatewayMessengerFacet.sol b/contracts/contracts/gateway/GatewayMessengerFacet.sol index 727e04d09..a658c3f40 100644 --- a/contracts/contracts/gateway/GatewayMessengerFacet.sol +++ b/contracts/contracts/gateway/GatewayMessengerFacet.sol @@ -45,7 +45,7 @@ contract GatewayMessengerFacet is GatewayActorModifiers { } // We prevent the sender from being an EoA. - if (!(msg.sender.code.length > 0)) { + if (msg.sender.code.length == 0) { revert InvalidXnetMessage(InvalidXnetMessageReason.Sender); } diff --git a/contracts/contracts/lib/LibGateway.sol b/contracts/contracts/lib/LibGateway.sol index b1cfc67ce..f7014348f 100644 --- a/contracts/contracts/lib/LibGateway.sol +++ b/contracts/contracts/lib/LibGateway.sol @@ -423,7 +423,7 @@ library LibGateway { sendReceipt(crossMsg, OutcomeType.SystemErr, abi.encodeWithSelector(InvalidXnetMessage.selector, InvalidXnetMessageReason.Nonce)); return; } - subnet.appliedBottomUpNonce += 1; + ++subnet.appliedBottomUpNonce; // The value carried in bottom-up messages needs to be treated according to the supply source // configuration of the subnet. @@ -434,7 +434,7 @@ library LibGateway { sendReceipt(crossMsg, OutcomeType.SystemErr, abi.encodeWithSelector(InvalidXnetMessage.selector, InvalidXnetMessageReason.Nonce)); return; } - s.appliedTopDownNonce += 1; + ++s.appliedTopDownNonce; // The value carried in top-down messages locally maps to the native coin, so we pass over the // native supply source. diff --git a/contracts/contracts/lib/LibSubnetActor.sol b/contracts/contracts/lib/LibSubnetActor.sol index 14731f768..bd1d43f09 100644 --- a/contracts/contracts/lib/LibSubnetActor.sol +++ b/contracts/contracts/lib/LibSubnetActor.sol @@ -120,7 +120,7 @@ library LibSubnetActor { uint256 length = validators.length; - if (length <= s.minValidators) { + if (length < s.minValidators) { revert NotEnoughGenesisValidators(); } diff --git a/contracts/contracts/subnet/SubnetActorManagerFacet.sol b/contracts/contracts/subnet/SubnetActorManagerFacet.sol index 144295158..a4ed782d9 100644 --- a/contracts/contracts/subnet/SubnetActorManagerFacet.sol +++ b/contracts/contracts/subnet/SubnetActorManagerFacet.sol @@ -24,6 +24,9 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa using LibValidatorSet for ValidatorSet; using Address for address payable; + event ValidatorGaterUpdated(address oldGater, address newGater); + event NewBootstrapNode(string netAddress, address owner); + /// @notice method to add some initial balance into a subnet that hasn't yet bootstrapped. /// @dev This balance is added to user addresses in genesis, and becomes part of the genesis /// circulating supply. @@ -73,8 +76,13 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa } } + /// @notice Sets the validator gater contract implementation + /// @param gater The addresseof validator gater implementation. function setValidatorGater(address gater) external notKilled { LibDiamond.enforceIsContractOwner(); + + emit ValidatorGaterUpdated(s.validatorGater, gater); + s.validatorGater = gater; } @@ -255,7 +263,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa // check if the validator had some initial balance and return it if not bootstrapped uint256 genesisBalance = s.genesisBalance[msg.sender]; if (genesisBalance != 0) { - s.genesisBalance[msg.sender] == 0; + delete s.genesisBalance[msg.sender]; s.genesisCircSupply -= genesisBalance; LibSubnetActor.rmAddressFromBalanceKey(msg.sender); s.collateralSource.transferFunds(payable(msg.sender), genesisBalance); @@ -284,7 +292,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa /// @notice Add a bootstrap node. /// @param netAddress The network address of the new bootstrap node. - function addBootstrapNode(string memory netAddress) external whenNotPaused { + function addBootstrapNode(string calldata netAddress) external whenNotPaused { if (!s.validatorSet.isActiveValidator(msg.sender)) { revert NotValidator(msg.sender); } @@ -294,5 +302,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa s.bootstrapNodes[msg.sender] = netAddress; // slither-disable-next-line unused-return s.bootstrapOwners.add(msg.sender); + + emit NewBootstrapNode(netAddress, msg.sender); } } diff --git a/contracts/test/integration/SubnetActorDiamond.t.sol b/contracts/test/integration/SubnetActorDiamond.t.sol index 3c80b43d0..c08e78f66 100644 --- a/contracts/test/integration/SubnetActorDiamond.t.sol +++ b/contracts/test/integration/SubnetActorDiamond.t.sol @@ -1584,7 +1584,7 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase { gatewayAddress, ConsensusType.Fendermint, DEFAULT_MIN_VALIDATOR_STAKE, - DEFAULT_MIN_VALIDATORS, + 2, DEFAULT_CHECKPOINT_PERIOD, DEFAULT_MAJORITY_PERCENTAGE, PermissionMode.Federated, From b3c1eee9674cd456542091d276a325cea3750440 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Mon, 20 Jan 2025 20:19:43 +0800 Subject: [PATCH 2/4] format --- contracts/contracts/gateway/GatewayManagerFacet.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/contracts/gateway/GatewayManagerFacet.sol b/contracts/contracts/gateway/GatewayManagerFacet.sol index 0e9c0f4f3..a026e9e02 100644 --- a/contracts/contracts/gateway/GatewayManagerFacet.sol +++ b/contracts/contracts/gateway/GatewayManagerFacet.sol @@ -63,7 +63,7 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { } /// @notice addStake - add collateral for an existing subnet - function addStake(uint256 amount) external nonReentrant payable { + function addStake(uint256 amount) external payable nonReentrant { if (amount == 0) { revert NotEnoughFunds(); } From 49b9ec2dc10b22fffd34c48b8b0d0b006241a084 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Tue, 21 Jan 2025 11:31:23 +0800 Subject: [PATCH 3/4] review feedback --- contracts/contracts/gateway/GatewayManagerFacet.sol | 4 ++-- contracts/contracts/lib/LibGateway.sol | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/contracts/gateway/GatewayManagerFacet.sol b/contracts/contracts/gateway/GatewayManagerFacet.sol index a026e9e02..bfbd2d71e 100644 --- a/contracts/contracts/gateway/GatewayManagerFacet.sol +++ b/contracts/contracts/gateway/GatewayManagerFacet.sol @@ -27,7 +27,7 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { using AssetHelper for Asset; using EnumerableSet for EnumerableSet.Bytes32Set; - event SubnetKilled(SubnetID id); + event SubnetDestroyed(SubnetID id); /// @notice register a subnet in the gateway. It is called by a subnet when it reaches the threshold stake /// @dev The subnet can optionally pass a genesis circulating supply that would be pre-allocated in the @@ -128,7 +128,7 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { s.subnetKeys.remove(id); SubnetActorGetterFacet(msg.sender).collateralSource().transferFunds(payable(msg.sender), stake); - emit SubnetKilled(subnet.id); + emit SubnetDestroyed(subnet.id); } /// @notice credits the received value to the specified address in the specified child subnet. diff --git a/contracts/contracts/lib/LibGateway.sol b/contracts/contracts/lib/LibGateway.sol index f7014348f..b1cfc67ce 100644 --- a/contracts/contracts/lib/LibGateway.sol +++ b/contracts/contracts/lib/LibGateway.sol @@ -423,7 +423,7 @@ library LibGateway { sendReceipt(crossMsg, OutcomeType.SystemErr, abi.encodeWithSelector(InvalidXnetMessage.selector, InvalidXnetMessageReason.Nonce)); return; } - ++subnet.appliedBottomUpNonce; + subnet.appliedBottomUpNonce += 1; // The value carried in bottom-up messages needs to be treated according to the supply source // configuration of the subnet. @@ -434,7 +434,7 @@ library LibGateway { sendReceipt(crossMsg, OutcomeType.SystemErr, abi.encodeWithSelector(InvalidXnetMessage.selector, InvalidXnetMessageReason.Nonce)); return; } - ++s.appliedTopDownNonce; + s.appliedTopDownNonce += 1; // The value carried in top-down messages locally maps to the native coin, so we pass over the // native supply source. From 5869e0bf377fa9ef3bda388b88904f8f15aefad7 Mon Sep 17 00:00:00 2001 From: cryptoAtwill Date: Thu, 23 Jan 2025 14:57:09 +0800 Subject: [PATCH 4/4] minor fixes --- contracts/contracts/gateway/GatewayManagerFacet.sol | 2 +- contracts/contracts/subnet/SubnetActorManagerFacet.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/contracts/gateway/GatewayManagerFacet.sol b/contracts/contracts/gateway/GatewayManagerFacet.sol index bfbd2d71e..7674de73e 100644 --- a/contracts/contracts/gateway/GatewayManagerFacet.sol +++ b/contracts/contracts/gateway/GatewayManagerFacet.sol @@ -63,7 +63,7 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard { } /// @notice addStake - add collateral for an existing subnet - function addStake(uint256 amount) external payable nonReentrant { + function addStake(uint256 amount) external payable { if (amount == 0) { revert NotEnoughFunds(); } diff --git a/contracts/contracts/subnet/SubnetActorManagerFacet.sol b/contracts/contracts/subnet/SubnetActorManagerFacet.sol index a4ed782d9..410717b3a 100644 --- a/contracts/contracts/subnet/SubnetActorManagerFacet.sol +++ b/contracts/contracts/subnet/SubnetActorManagerFacet.sol @@ -77,7 +77,7 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa } /// @notice Sets the validator gater contract implementation - /// @param gater The addresseof validator gater implementation. + /// @param gater The addresse of validator gater implementation. function setValidatorGater(address gater) external notKilled { LibDiamond.enforceIsContractOwner();