[TD-1397] ImmutableSignedZoneV2 internal audit fixes (#214)

* update readme Signed-off-by: Frank Li <[email protected]> * pin solidity version Signed-off-by: Frank Li <[email protected]> * perform check before state change Signed-off-by: Frank Li <[email protected]> * pin ImmutableSignedZoneV2 contracts to 0.8.20 * Revert if context is empty for ImmutableSignedZoneV2 * Revert "pin ImmutableSignedZoneV2 contracts to 0.8.20" This reverts commit 8da6123. * Restore pin * Disallow revoking role if the role member count is currently 1 * remove trailing comma * documentation rework * fix test Signed-off-by: Frank Li <[email protected]> * fix regression Signed-off-by: Frank Li <[email protected]> * update test name Signed-off-by: Frank Li <[email protected]> * add audit file Signed-off-by: Frank Li <[email protected]> * Update contracts/trading/seaport/zones/immutable-signed-zone/v2/README.md --------- Signed-off-by: Frank Li <[email protected]> Co-authored-by: Frank Li <[email protected]> Co-authored-by: Naveen <[email protected]>
immutable · May 6, 2024 · 15c39af · 15c39af
1 parent d45c52d
commit 15c39af
Show file tree

Hide file tree

Showing 6 changed files with 140 additions and 21 deletions.
diff --git a/audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf b/audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf
diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol
@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2
 
 // solhint-disable-next-line compiler-version
-pragma solidity ^0.8.20;
+pragma solidity 0.8.20;
 
 import {AccessControlEnumerable} from "openzeppelin-contracts-5.0.2/access/extensions/AccessControlEnumerable.sol";
 import {ECDSA} from "openzeppelin-contracts-5.0.2/utils/cryptography/ECDSA.sol";
@@ -22,6 +22,10 @@ import {SIP7Interface} from "./interfaces/SIP7Interface.sol";
  * @notice ImmutableSignedZoneV2 is a zone implementation based on the
  *         SIP-7 standard https://github.com/ProjectOpenSea/SIPs/blob/main/SIPS/sip-7.md
  *         implementing substandards 3, 4 and 6.
+ * 
+ *         The contract is not upgradable. If the contract needs to be changed a new version
+ *         should be deployed, and the old version should be removed from the Seaport contract
+ *         zone allowlist.
  */
 contract ImmutableSignedZoneV2 is
     ERC165,
@@ -397,10 +401,14 @@ contract ImmutableSignedZoneV2 is
         uint256 startIndex = 0;
         uint256 contextLength = context.length;
 
+        // The ImmutableSignedZoneV2 contract enforces at least
+        // one of the supported substandards is present in the context.
+        if (contextLength == 0) {
+            revert InvalidExtraData("invalid context, no substandards present", zoneParameters.orderHash);
+        }
+
         // Each _validateSubstandard* function returns the length of the substandard
         // segment (0 if the substandard was not matched).
-
-        if (startIndex == contextLength) return;
         startIndex = _validateSubstandard3(context[startIndex:], zoneParameters) + startIndex;
 
         if (startIndex == contextLength) return;

diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/README.md b/contracts/trading/seaport/zones/immutable-signed-zone/v2/README.md
@@ -13,7 +13,7 @@ Contract threat models and audits:
 
 | Description                     | Date | Version Audited | Link to Report |
 | ------------------------------- | ---- | --------------- | -------------- |
-| Not audited and no threat model | -    | -               | -              |
+| Not audited and no threat model | 2024-05-02   | V2               | -  ../../../audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf            |
 
 ## ImmutableSignedZoneV2
 
@@ -46,4 +46,18 @@ The sequence of events is as follows:
 2. The client calls `fulfillAdvancedOrder` or `fulfillAvailableAdavancedOrders` on `ImmutableSeaport.sol` to fulfill an order
 3. `ImmutableSeaport.sol` executes the fufilment by transferring items between parties
 4. `ImmutableSeaport.sol` calls `validateOrder` on `ImmutableSignedZoneV2.sol`, passing it the fulfilment execution details as well as the `extraData` parameter
-   1. `ImmutableSignedZoneV2.sol` validates the fulfilment execution details using the `extraData` payload, reverting if expectations are not met
+5. `ImmutableSignedZoneV2.sol` validates the fulfilment execution details using the `extraData` payload, reverting if expectations are not met
+
+## Differences compared to ImmutableSignedZone (v1)
+
+The contract was developed based on ImmutableSignedZone, with the addition of:
+ - SIP7 substandard 6 support
+ - Role based access control to be role based
+
+### ZoneAccessControl
+
+The contract now uses a finer grained access control with role based access with the `ZoneAccessControl` interface, rather than the `Ownable` interface in the v1 contract. A seperate `zoneManager` roles is used to manage signers and an admin role used to control roles.
+
+### Support of SIP7 substandard 6
+
+The V2 contract now supports substandard-6 of the SIP7 specification, found here (https://github.com/immutable/platform-services/pull/12775). A server side signed order can adhere to substandard 3 + 4 (full fulfillment only) or substandard 6 + 4 (full or partial fulfillment).
diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ZoneAccessControl.sol
@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2
 
 // solhint-disable-next-line compiler-version
-pragma solidity ^0.8.20;
+pragma solidity 0.8.20;
 
 import {AccessControl} from "openzeppelin-contracts-5.0.2/access/AccessControl.sol";
 import {IAccessControl} from "openzeppelin-contracts-5.0.2/access/IAccessControl.sol";
@@ -31,21 +31,21 @@ abstract contract ZoneAccessControl is AccessControlEnumerable, ZoneAccessContro
      * @inheritdoc AccessControl
      */
     function revokeRole(bytes32 role, address account) public override(AccessControl, IAccessControl) onlyRole(getRoleAdmin(role)) {
-        super.revokeRole(role, account);
-
-        if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 0) {
+        if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 1) {
             revert LastDefaultAdminRole(account);
         }
+
+        super.revokeRole(role, account);
     }
 
     /**
      * @inheritdoc AccessControl
      */
     function renounceRole(bytes32 role, address callerConfirmation) public override(AccessControl, IAccessControl) {
-        super.renounceRole(role, callerConfirmation);
-
-        if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 0) {
+        if (role == DEFAULT_ADMIN_ROLE && super.getRoleMemberCount(DEFAULT_ADMIN_ROLE) == 1) {
             revert LastDefaultAdminRole(callerConfirmation);
         }
+
+        super.renounceRole(role, callerConfirmation);
     }
 }
diff --git a/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol b/test/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.t.sol
@@ -532,20 +532,45 @@ contract ImmutableSignedZoneV2Test is
 
     function test_validateOrder_revertsIfSignerIsNotActive() public {
         ImmutableSignedZoneV2Harness zone = _newZoneHarness(OWNER);
+        // no signer added
+
         bytes32 orderHash = bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9);
         uint64 expiration = 100;
 
-        bytes memory extraData =
-            _buildExtraData(zone, SIGNER_PRIVATE_KEY, FULFILLER, expiration, orderHash, new bytes(0));
+        SpentItem[] memory spentItems = new SpentItem[](1);
+        spentItems[0] = SpentItem({itemType: ItemType.ERC1155, token: address(0x5), identifier: 222, amount: 10});
+
+        ReceivedItem[] memory receivedItems = new ReceivedItem[](1);
+        ReceivedItem memory receivedItem = ReceivedItem({
+            itemType: ItemType.ERC20,
+            token: address(0x4),
+            identifier: 0,
+            amount: 20,
+            recipient: payable(address(0x3))
+        });
+        receivedItems[0] = receivedItem;
+
+        bytes32[] memory orderHashes = new bytes32[](1);
+        orderHashes[0] = bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9);
+
+        // console.logBytes32(zone.exposed_deriveReceivedItemsHash(receivedItems, 1, 1));
+        bytes32 substandard3Data = bytes32(0xec07a42041c18889c5c5dcd348923ea9f3d0979735bd8b3b687ebda38d9b6a31);
+        bytes memory substandard4Data = abi.encode(orderHashes);
+        bytes memory substandard6Data = abi.encodePacked(uint256(10), substandard3Data);
+        bytes memory context = abi.encodePacked(
+            bytes1(0x03), substandard3Data, bytes1(0x04), substandard4Data, bytes1(0x06), substandard6Data
+        );
+
+        bytes memory extraData = _buildExtraData(zone, SIGNER_PRIVATE_KEY, FULFILLER, expiration, orderHash, context);
 
         ZoneParameters memory zoneParameters = ZoneParameters({
             orderHash: bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9),
             fulfiller: FULFILLER,
             offerer: OFFERER,
-            offer: new SpentItem[](0),
-            consideration: new ReceivedItem[](0),
+            offer: spentItems,
+            consideration: receivedItems,
             extraData: extraData,
-            orderHashes: new bytes32[](0),
+            orderHashes: orderHashes,
             startTime: 0,
             endTime: 0,
             zoneHash: bytes32(0)
@@ -556,6 +581,54 @@ contract ImmutableSignedZoneV2Test is
         zone.validateOrder(zoneParameters);
     }
 
+    function test_validateOrder_revertsIfContextIsEmpty() public {
+        ImmutableSignedZoneV2Harness zone = _newZoneHarness(OWNER);
+        bytes32 managerRole = zone.ZONE_MANAGER_ROLE();
+        vm.prank(OWNER);
+        zone.grantRole(managerRole, OWNER);
+        vm.prank(OWNER);
+        zone.addSigner(SIGNER);
+
+        bytes32 orderHash = bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9);
+        uint64 expiration = 100;
+
+        SpentItem[] memory spentItems = new SpentItem[](1);
+        spentItems[0] = SpentItem({itemType: ItemType.ERC1155, token: address(0x5), identifier: 222, amount: 10});
+
+        ReceivedItem[] memory receivedItems = new ReceivedItem[](1);
+        ReceivedItem memory receivedItem = ReceivedItem({
+            itemType: ItemType.ERC20,
+            token: address(0x4),
+            identifier: 0,
+            amount: 20,
+            recipient: payable(address(0x3))
+        });
+        receivedItems[0] = receivedItem;
+
+        bytes32[] memory orderHashes = new bytes32[](1);
+        orderHashes[0] = bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9);
+
+        bytes memory extraData = _buildExtraDataWithoutContext(zone, SIGNER_PRIVATE_KEY, FULFILLER, expiration, orderHash, new bytes(0));
+
+        ZoneParameters memory zoneParameters = ZoneParameters({
+            orderHash: bytes32(0x43592598d0419e49d268e9b553427fd7ba1dd091eaa3f6127161e44afb7b40f9),
+            fulfiller: FULFILLER,
+            offerer: OFFERER,
+            offer: spentItems,
+            consideration: receivedItems,
+            extraData: extraData,
+            orderHashes: orderHashes,
+            startTime: 0,
+            endTime: 0,
+            zoneHash: bytes32(0)
+        });
+
+        vm.expectRevert(
+            abi.encodeWithSelector(InvalidExtraData.selector, "invalid context, no substandards present", zoneParameters.orderHash)
+        );
+        zone.validateOrder(zoneParameters);
+    }
+
     function test_validateOrder_returnsMagicValueOnSuccessfulValidation() public {
         ImmutableSignedZoneV2Harness zone = _newZoneHarness(OWNER);
         bytes32 managerRole = zone.ZONE_MANAGER_ROLE();
@@ -671,8 +744,7 @@ contract ImmutableSignedZoneV2Test is
     }
 
     /* _validateSubstandards */
-
-    function test_validateSubstandards_emptyContext() public {
+    function test_validateSubstandards_revertsIfEmptyContext() public {
         ImmutableSignedZoneV2Harness zone = _newZoneHarness(OWNER);
 
         ZoneParameters memory zoneParameters = ZoneParameters({
@@ -688,6 +760,12 @@ contract ImmutableSignedZoneV2Test is
             zoneHash: bytes32(0)
         });
 
+         vm.expectRevert(
+            abi.encodeWithSelector(
+                InvalidExtraData.selector, "invalid context, no substandards present", zoneParameters.orderHash
+            )
+        );
+
         zone.exposed_validateSubstandards(new bytes(0), zoneParameters);
     }
 
@@ -1435,6 +1513,24 @@ contract ImmutableSignedZoneV2Test is
         );
         return extraData;
     }
+
+    function _buildExtraDataWithoutContext(
+        ImmutableSignedZoneV2Harness zone,
+        uint256 signerPrivateKey,
+        address fulfiller,
+        uint64 expiration,
+        bytes32 orderHash,
+        bytes memory context
+    ) private view returns (bytes memory) {
+        bytes32 eip712SignedOrderHash = zone.exposed_deriveSignedOrderHash(fulfiller, expiration, orderHash, context);
+        bytes memory extraData = abi.encodePacked(
+            bytes1(0),
+            fulfiller,
+            expiration,
+            _signCompact(signerPrivateKey, ECDSA.toTypedDataHash(zone.exposed_domainSeparator(), eip712SignedOrderHash))
+        );
+        return extraData;
+    }
 }
 
 // solhint-enable func-name-mixedcase
diff --git a/test/trading/seaport/zones/immutable-signed-zone/v2/README.md b/test/trading/seaport/zones/immutable-signed-zone/v2/README.md
@@ -50,6 +50,7 @@ Operational function tests:
 | `test_validateOrder_revertsIfActualFulfillerDoesNotMatchExpectedFulfiller` | Validate order with unexpected fufiller.           | No         | Yes         |
 | `test_validateOrder_revertsIfActualFulfillerDoesNotMatchExpectedFulfiller` | Validate order with expected *any* fufiller.       | Yes        | No          |
 | `test_validateOrder_revertsIfSignerIsNotActive`                            | Validate order with inactive signer.               | No         | Yes         |
+| `test_validateOrder_revertsIfContextIsEmpty`                               | Validate order with an empty context.              | No         | Yes         |
 | `test_validateOrder_returnsMagicValueOnSuccessfulValidation`               | Validate order successfully.                       | Yes        | Yes         |
 
 Internal operational function tests:
@@ -61,7 +62,7 @@ Internal operational function tests:
 | `test_deriveDomainSeparator_returnsDomainSeparatorForChainID`                                    | Domain separator derivation.                                                                 | Yes        | Yes         |
 | `test_getSupportedSubstandards`                                                                  | Retrieve Zone's supported substandards.                                                      | Yes        | Yes         |
 | `test_deriveSignedOrderHash_returnsHashOfSignedOrder`                                            | Signed order hash derivation.                                                                | Yes        | Yes         |
-| `test_validateSubstandards_emptyContext`                                                         | Empty context without substandards.                                                          | Yes        | Yes         |
+| `test_validateSubstandards_revertsIfEmptyContext`                                                | Empty context without substandards should revert.                                            | No         | Yes         |
 | `test_validateSubstandards_substandard3`                                                         | Context with substandard 3.                                                                  | Yes        | Yes         |
 | `test_validateSubstandards_substandard4`                                                         | Context with substandard 4.                                                                  | Yes        | Yes         |
 | `test_validateSubstandards_substandard6`                                                         | Context with substandard 6.                                                                  | Yes        | Yes         |
@@ -98,4 +99,4 @@ All of these tests are in [test/trading/seaport/ImmutableSeaportSignedZoneV2Inte
 | `test_fulfillAdvancedOrder_withCompleteFulfilment`   | Full fulfilment.                | Yes        | Yes         |
 | `test_fulfillAdvancedOrder_withPartialFill`          | Partial fulfilment.             | Yes        | Yes         |
 | `test_fulfillAdvancedOrder_withMultiplePartialFills` | Sequential partial fulfilments. | Yes        | Yes         |
-| `test_fulfillAdvancedOrder_withOverfilling`          | Over fulfilment.                | Yes        | Yes         |
+| `test_fulfillAdvancedOrder_withOverfilling`          | Over fulfilment.                | Yes        | Yes         |