Disable L3

[cryptoAtwill]

Reenable disable L3 subnet.

[dnkolegov]

We had an issue from the auditors related to this:

In the register() function (GatewayManagerFacet.sol L32) ,it is checked that route length after the addition of the subnet i.e. route.length + 1 must not exceed maxTreeDepth.
But this condition would revert even when new length after addition is equal to maxTreeDepth
while the value maxTreeDepth should be allowed . 

Recommendation:

if (s.networkName.route.length + 1 > s.maxTreeDepth) {
            revert MethodNotAllowed(ERR_CHILD_SUBNET_NOT_ALLOWED);
        }

WDYT?

[cryptoAtwill]

so the concern is probably the check is wrong?

[dnkolegov]

Yes, they suggest using > instead of >=

[maciejwitowski]

Yeah, I think > should be used. Wanted to check on a few cases below.
@cryptoAtwill could you add a unit test(s) for this as part of this PR?

• Case 1:

  • We allow L2 so s.maxTreeDepth == 2
  • Someone tries to register L2: the route is r123/abcd so s.networkName.route.length is 1 (abcd)
  • This check is then 1 + 1 > 2
  • The condition isn't met so it DOESN'T revert

• Case 2:

  • We allow L2 so s.maxTreeDepth == 2
  • Someone tries to register L3: the route is r123/abcd/efgh so s.networkName.route.length is 2 (abcd/efgh)
  • This check is then 3 + 1 > 2
  • The condition is met so it DOES revert

• Case 3: Let's say L3 is allowed -> s.maxTreeDepth == 3

  • The route for L3 is r123/abcd/efgh so s.networkName.route.length is 2 (abcd/efgh)
  • This check is then 2 + 1 > 3
  • The condition isn't met so it DOESN't revert

[cryptoAtwill]

I think this check is correct. For L1, the route.length is 0, for L2 the route.length is 1. s.maxTreeDepth is default to 2. So to create L3, we will have L2's route.length + 1 which is 2 and equals s.maxTreeDepth.

[cryptoAtwill]

@dnkolegov @maciejwitowski I think your examples mixed subnetId with s.networkName. s.networkName is the current subnet and subnetId is the subnet to be created. This check is sent to the parent.

[cryptoAtwill]

@dnkolegov @maciejwitowski I think your examples mixed subnetId with s.networkName. s.networkName is the current subnet and subnetId is the subnet to be created. This check is sent to the parent.

[maciejwitowski]

I think your examples mixed subnetId with s.networkName. s.networkName is the current subnet and subnetId is the subnet to be created. This check is sent to the parent.

Oh, I missed the fact this if is before the s.networkName.createSubnetId so in flow of creating L3 the s.networkName.route.length is the one for L2 (which is 1). You may be right then 😄
Anyways, adding a few unit tests with clear names like revert_L3_creation_when_it_is_disabled which test the edge conditions of >= would be recommended here I think.

[aakoshh]

This got me too as well in the past. Comments would be good because it looks like there is nobody who has looked at this and can tell what it does.