diff --git a/audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf b/audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf new file mode 100644 index 00000000..84299495 Binary files /dev/null and b/audits/trading/202405-internal-audit-immutable-signed-zone-v2.pdf differ diff --git a/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol b/contracts/trading/seaport/zones/immutable-signed-zone/v2/ImmutableSignedZoneV2.sol index b22dd795..5d79556b 100644 --- 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 index 13863d78..368fd0a6 100644 --- 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 \ No newline at end of file +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 index 19a5c50b..f45c11a2 100644 --- 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 index 2cb42bf2..7dabed13 100644 --- 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 index a5dc1cc8..7c59b662 100644 --- 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 | \ No newline at end of file +| `test_fulfillAdvancedOrder_withOverfilling` | Over fulfilment. | Yes | Yes |