Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(contracts): fix contract audit findings #1254

Merged
merged 5 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions contracts/contracts/gateway/GatewayManagerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
using AssetHelper for Asset;
using EnumerableSet for EnumerableSet.Bytes32Set;

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
/// subnet from genesis (without having to wait for the subnet to be spawned to propagate the funds).
Expand All @@ -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);
Expand All @@ -58,6 +53,13 @@ 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
Expand All @@ -72,7 +74,6 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
SubnetActorGetterFacet(msg.sender).collateralSource().lock(amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not move the lock after the check?


(bool registered, Subnet storage subnet) = LibGateway.getSubnet(msg.sender);

if (!registered) {
revert NotRegisteredSubnet();
}
Expand Down Expand Up @@ -126,6 +127,8 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {

s.subnetKeys.remove(id);
SubnetActorGetterFacet(msg.sender).collateralSource().transferFunds(payable(msg.sender), stake);

emit SubnetDestroyed(subnet.id);
}

/// @notice credits the received value to the specified address in the specified child subnet.
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/gateway/GatewayMessengerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

revert InvalidXnetMessage(InvalidXnetMessageReason.Sender);
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/lib/LibSubnetActor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ library LibSubnetActor {

uint256 length = validators.length;

if (length <= s.minValidators) {
if (length < s.minValidators) {
revert NotEnoughGenesisValidators();
}

Expand Down
14 changes: 12 additions & 2 deletions contracts/contracts/subnet/SubnetActorManagerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -73,8 +76,13 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
}
}

/// @notice Sets the validator gater contract implementation
/// @param gater The addresse of validator gater implementation.
function setValidatorGater(address gater) external notKilled {
LibDiamond.enforceIsContractOwner();

emit ValidatorGaterUpdated(s.validatorGater, gater);

s.validatorGater = gater;
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
}
2 changes: 1 addition & 1 deletion contracts/test/integration/SubnetActorDiamond.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -1584,7 +1584,7 @@ contract SubnetActorDiamondTest is Test, IntegrationTestBase {
gatewayAddress,
ConsensusType.Fendermint,
DEFAULT_MIN_VALIDATOR_STAKE,
DEFAULT_MIN_VALIDATORS,
2,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend assigning this to a constant variable for readability

DEFAULT_CHECKPOINT_PERIOD,
DEFAULT_MAJORITY_PERCENTAGE,
PermissionMode.Federated,
Expand Down
Loading