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.

Sign up for GitHub

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

Disable L3 #678

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Disable L3 #678

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions contracts/src/gateway/GatewayManagerFacet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ contract GatewayManagerFacet is GatewayActorModifiers, ReentrancyGuard {
function register(uint256 genesisCircSupply) external payable {
// If L2+ support is not enabled, only allow the registration of new
// subnets in the root
// if (s.networkName.route.length + 1 >= s.maxTreeDepth) {
// revert MethodNotAllowed(ERR_CHILD_SUBNET_NOT_ALLOWED);
// }
if (s.networkName.route.length + 1 >= s.maxTreeDepth) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the concern is probably the check is wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they suggest using > instead of >=

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

revert MethodNotAllowed(ERR_CHILD_SUBNET_NOT_ALLOWED);
}

if (msg.value < genesisCircSupply) {
revert NotEnoughFunds();
Expand Down
Loading