fix(contracts): fix contract audit findings #1254

cryptoAtwill · 2025-01-20T10:24:48Z

This PR addresses the review audits.

Fixes

Mediums:

M1: Addressed

Lows:

L1: I dont think we should address this. The subnet does not really know if the destination subnet exists. Caller should be aware.
L2: Updated, this one is known to have created tons of confusion.

Informationals:

To discuss

L3: We should taggle gas_limit in cross message transactions, but the issue is more complex than described. Maybe delay to another issue?
I4: We should discuss if we need this, but I think it’s a good practice
I7: I dont see the line mentioned, maybe the codebase is different

karlem · 2025-01-20T12:26:51Z

@cryptoAtwill I think it might be useful to add a sentence to each point in the list if possible. It is not clear from here what is being fixed.

raulk · 2025-01-20T12:26:14Z

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);


Let's name this event SubnetDestroyed. That term is more general as it can apply in ephemeral subnets, self-destruction, expirations (e.g. subnet is not bootstrapped in a predefined amount of time).

raulk · 2025-01-20T12:30:45Z

contracts/contracts/gateway/GatewayManagerFacet.sol

    }

    /// @notice addStake - add collateral for an existing subnet
-    function addStake(uint256 amount) external payable {
+    function addStake(uint256 amount) external payable nonReentrant {


This is fine, but is there really a re-entrancy risk here? The contract call is performed before we update credit the stake, so it shouldn't bear any risks.

lock does the check already, this then can go.

raulk · 2025-01-20T12:31:17Z

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) {


raulk · 2025-01-20T12:32:20Z

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;


Given that this is purely stylistic, I would use a post-increment. It's visually more common.

@cryptoAtwill Can't we do appliedBottomUpNonce++?

I think ++appliedBottomUpNonce is appliedBottomUpNonce += 1 but appliedBottomUpNonce++ is actually:

i = appliedBottomUpNonce; appliedBottomUpNonce = appliedBottomUpNonce + 1; i

it's slightly more gas consuming.

karlem · 2025-01-22T15:00:57Z

contracts/contracts/gateway/GatewayManagerFacet.sol

@@ -72,7 +74,6 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
        SubnetActorGetterFacet(msg.sender).collateralSource().lock(amount);


Should we not move the lock after the check?

karlem · 2025-01-22T15:02:16Z

contracts/contracts/subnet/SubnetActorManagerFacet.sol

@@ -73,8 +76,13 @@ contract SubnetActorManagerFacet is SubnetActorModifiers, ReentrancyGuard, Pausa
        }
    }

+    /// @notice Sets the validator gater contract implementation
+    /// @param gater The addresseof validator gater implementation.


NIT: addresseof -> address of

karlem

Just small nits.

aiagentdev-1990

I left a small comment but this looks good overall. Also consider adding additional unit tests to cover some of the fixes.

aiagentdev-1990 · 2025-01-23T10:36:11Z

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,


I recommend assigning this to a constant variable for readability

update contract audit

4b9ae39

cryptoAtwill requested a review from a team as a code owner January 20, 2025 10:24

format

b3c1eee

raulk changed the title ~~fix(node): update contract audit~~ fix(contracts): update contract audit Jan 20, 2025

raulk changed the title ~~fix(contracts): update contract audit~~ fix(contracts): fix contract audit findings Jan 20, 2025

raulk approved these changes Jan 20, 2025

View reviewed changes

review feedback

49b9ec2

karlem reviewed Jan 22, 2025

View reviewed changes

Merge branch 'main' into fix-audit

fa01590

karlem approved these changes Jan 22, 2025

View reviewed changes

minor fixes

5869e0b

aiagentdev-1990 reviewed Jan 23, 2025

View reviewed changes

cryptoAtwill merged commit c455add into main Jan 23, 2025
15 checks passed

cryptoAtwill deleted the fix-audit branch January 23, 2025 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(contracts): fix contract audit findings #1254

fix(contracts): fix contract audit findings #1254

cryptoAtwill commented Jan 20, 2025 •

edited by raulk

Loading

karlem commented Jan 20, 2025

raulk Jan 20, 2025

raulk Jan 20, 2025

cryptoAtwill Jan 21, 2025

raulk Jan 20, 2025

raulk Jan 20, 2025

karlem Jan 22, 2025

cryptoAtwill Jan 23, 2025

karlem Jan 22, 2025

karlem Jan 22, 2025 •

edited

Loading

karlem left a comment

aiagentdev-1990 left a comment

aiagentdev-1990 Jan 23, 2025

		@@ -72,7 +74,6 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
		SubnetActorGetterFacet(msg.sender).collateralSource().lock(amount);

fix(contracts): fix contract audit findings #1254

fix(contracts): fix contract audit findings #1254

Conversation

cryptoAtwill commented Jan 20, 2025 • edited by raulk Loading

Fixes

To discuss

karlem commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlem Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

karlem left a comment

Choose a reason for hiding this comment

aiagentdev-1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoAtwill commented Jan 20, 2025 •

edited by raulk

Loading

karlem Jan 22, 2025 •

edited

Loading