From 4d9654bcf3104f5bd08f0a2c843b3794c3e4ade4 Mon Sep 17 00:00:00 2001 From: Andres Adjimann Date: Tue, 25 Jun 2024 12:35:38 -0300 Subject: [PATCH] fix: apply small changes after the audit - Add a few interfaces related to quad operations and metadata registry to suppportInteface - Make UNKNOWN_NEIGHBORHOOD constant private in the land contract so it is not exposed in the abi. - This PR include some minor changes that we are fixing in advance as a result of some comments from the auditors. If needed they will be split in separated PRs before merging this one. --- .../land/contracts/LandMetadataRegistry.sol | 6 ++-- packages/land/contracts/common/LandBase.sol | 9 ++++- .../contracts/common/WithMetadataRegistry.sol | 2 +- .../land/contracts/interfaces/ILandToken.sol | 29 ++-------------- packages/land/contracts/interfaces/IQuad.sol | 34 +++++++++++++++++++ .../contracts/registry/LandMetadataBase.sol | 22 ++++++------ packages/land/test/common/ERC721.behavior.ts | 19 +++++++++++ 7 files changed, 78 insertions(+), 43 deletions(-) create mode 100644 packages/land/contracts/interfaces/IQuad.sol diff --git a/packages/land/contracts/LandMetadataRegistry.sol b/packages/land/contracts/LandMetadataRegistry.sol index 4d43e4c3f8..5674f44251 100644 --- a/packages/land/contracts/LandMetadataRegistry.sol +++ b/packages/land/contracts/LandMetadataRegistry.sol @@ -19,7 +19,7 @@ contract LandMetadataRegistry is IErrors, ILandMetadataRegistry, AccessControlEn } struct BatchSetData { - // baseTokenId the token id floor 32 + // baseTokenId the token id floor LANDS_PER_WORD uint256 baseTokenId; // metadata: premiumness << 8 | neighborhoodId uint256 metadata; @@ -125,7 +125,7 @@ contract LandMetadataRegistry is IErrors, ILandMetadataRegistry, AccessControlEn } } - /// @notice set the metadata for 32 lands at the same time in batch + /// @notice set the metadata for LANDS_PER_WORD lands at the same time in batch /// @param data token id and metadata /// @dev use with care, we can set to the metadata for some lands to unknown (zero) function batchSetMetadata(BatchSetData[] calldata data) external onlyAdmin { @@ -183,7 +183,7 @@ contract LandMetadataRegistry is IErrors, ILandMetadataRegistry, AccessControlEn return _getNeighborhoodName(neighborhoodId); } - /// @notice return the metadata of 32 lands at once + /// @notice return the metadata of LANDS_PER_WORD lands at once /// @param tokenIds the token ids /// @return the raw metadata for a series of tokenIds /// @dev used to debug, extracting a lot of information that must be unpacked at once. diff --git a/packages/land/contracts/common/LandBase.sol b/packages/land/contracts/common/LandBase.sol index 0871d47066..31cfe1d117 100644 --- a/packages/land/contracts/common/LandBase.sol +++ b/packages/land/contracts/common/LandBase.sol @@ -8,6 +8,9 @@ import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import {IERC721Metadata} from "@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol"; import {IOperatorFilterRegistry} from "../interfaces/IOperatorFilterRegistry.sol"; import {IERC173} from "../interfaces/IERC173.sol"; +import {ILandToken} from "../interfaces/ILandToken.sol"; +import {IQuad} from "../interfaces/IQuad.sol"; +import {ILandMetadataRegistry} from "../interfaces/ILandMetadataRegistry.sol"; import {IERC721BatchOps} from "../interfaces/IERC721BatchOps.sol"; import {WithAdmin} from "./WithAdmin.sol"; import {OperatorFiltererUpgradeable} from "../common/OperatorFiltererUpgradeable.sol"; @@ -212,6 +215,10 @@ abstract contract LandBase is interfaceId == type(IERC721Metadata).interfaceId || interfaceId == type(IERC165).interfaceId || interfaceId == type(IERC173).interfaceId || - interfaceId == type(IERC2981).interfaceId; + interfaceId == type(IERC2981).interfaceId || + interfaceId == type(ILandToken).interfaceId || + interfaceId == type(ILandToken).interfaceId ^ type(IQuad).interfaceId || + interfaceId == type(IQuad).interfaceId || + interfaceId == type(ILandMetadataRegistry).interfaceId; } } diff --git a/packages/land/contracts/common/WithMetadataRegistry.sol b/packages/land/contracts/common/WithMetadataRegistry.sol index d5279c9abb..ff1c5db49c 100644 --- a/packages/land/contracts/common/WithMetadataRegistry.sol +++ b/packages/land/contracts/common/WithMetadataRegistry.sol @@ -11,7 +11,7 @@ import {ILandMetadataRegistry} from "../interfaces/ILandMetadataRegistry.sol"; /// @notice Add support for the metadata registry abstract contract WithMetadataRegistry is IErrors { /// @notice value returned when the neighborhood is not set yet. - string public constant UNKNOWN_NEIGHBORHOOD = "unknown"; + string private constant UNKNOWN_NEIGHBORHOOD = "unknown"; /// @notice emitted when the metadata registry is set /// @param metadataRegistry the address of the metadata registry diff --git a/packages/land/contracts/interfaces/ILandToken.sol b/packages/land/contracts/interfaces/ILandToken.sol index f99553981e..e595e63611 100644 --- a/packages/land/contracts/interfaces/ILandToken.sol +++ b/packages/land/contracts/interfaces/ILandToken.sol @@ -2,36 +2,13 @@ pragma solidity ^0.8.0; +import {IQuad} from "./IQuad.sol"; + /// @title ILandToken /// @author The Sandbox /// @custom:security-contact contact-blockchain@sandbox.game /// @notice Interface of the LAND token including quad methods -interface ILandToken { - /// @notice transfer multiple quad (aligned to a quad tree with size 3, 6, 12 or 24 only) - /// @param from current owner of the quad - /// @param to destination - /// @param sizes list of sizes for each quad - /// @param xs list of bottom left x coordinates for each quad - /// @param ys list of bottom left y coordinates for each quad - /// @param data additional data - function batchTransferQuad( - address from, - address to, - uint256[] calldata sizes, - uint256[] calldata xs, - uint256[] calldata ys, - bytes calldata data - ) external; - - /// @notice transfer one quad (aligned to a quad tree with size 3, 6, 12 or 24 only) - /// @param from current owner of the quad - /// @param to destination - /// @param size size of the quad - /// @param x The bottom left x coordinate of the quad - /// @param y The bottom left y coordinate of the quad - /// @param data additional data - function transferQuad(address from, address to, uint256 size, uint256 x, uint256 y, bytes calldata data) external; - +interface ILandToken is IQuad { /// @notice Mint a new quad (aligned to a quad tree with size 1, 3, 6, 12 or 24 only) /// @param to The recipient of the new quad /// @param size The size of the new quad diff --git a/packages/land/contracts/interfaces/IQuad.sol b/packages/land/contracts/interfaces/IQuad.sol new file mode 100644 index 0000000000..835a682c09 --- /dev/null +++ b/packages/land/contracts/interfaces/IQuad.sol @@ -0,0 +1,34 @@ +//SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +/// @title IQuad +/// @author The Sandbox +/// @custom:security-contact contact-blockchain@sandbox.game +/// @notice Interface of the LAND token (quad methods) +interface IQuad { + /// @notice transfer multiple quad (aligned to a quad tree with size 3, 6, 12 or 24 only) + /// @param from current owner of the quad + /// @param to destination + /// @param sizes list of sizes for each quad + /// @param xs list of bottom left x coordinates for each quad + /// @param ys list of bottom left y coordinates for each quad + /// @param data additional data + function batchTransferQuad( + address from, + address to, + uint256[] calldata sizes, + uint256[] calldata xs, + uint256[] calldata ys, + bytes calldata data + ) external; + + /// @notice transfer one quad (aligned to a quad tree with size 3, 6, 12 or 24 only) + /// @param from current owner of the quad + /// @param to destination + /// @param size size of the quad + /// @param x The bottom left x coordinate of the quad + /// @param y The bottom left y coordinate of the quad + /// @param data additional data + function transferQuad(address from, address to, uint256 size, uint256 x, uint256 y, bytes calldata data) external; +} diff --git a/packages/land/contracts/registry/LandMetadataBase.sol b/packages/land/contracts/registry/LandMetadataBase.sol index c9a89337bd..e6660a99ba 100644 --- a/packages/land/contracts/registry/LandMetadataBase.sol +++ b/packages/land/contracts/registry/LandMetadataBase.sol @@ -1,13 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.23; -import {AccessControlEnumerableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/extensions/AccessControlEnumerableUpgradeable.sol"; - /// @title LandMetadataRegistry /// @author The Sandbox /// @custom:security-contact contact-blockchain@sandbox.game /// @notice Store information about the lands (premiumness and neighborhood) -abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { +abstract contract LandMetadataBase { /// @notice the base token id used for a batch operation is wrong /// @param tokenId the id of the token error InvalidBaseTokenId(uint256 tokenId); @@ -30,7 +28,7 @@ abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { uint256 public constant NEIGHBORHOOD_MASK = PREMIUM_MASK - 1; struct LandMetadataStorage { - /// @dev tokenId / 32 => premiumness + neighborhood metadata + /// @dev tokenId / LANDS_PER_WORD => premiumness + neighborhood metadata /// @dev zero means no metadata definition mapping(uint256 tokenId => uint256 metadataType) _metadata; /// @dev neighborhood number to string mapping @@ -61,7 +59,7 @@ abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { } /// @notice get the packed metadata for a single land - /// @param tokenId the base token id floor 32 + /// @param tokenId the base token id floor LANDS_PER_WORD /// @return neighborhoodId the number that identifies the neighborhood /// @return premium true if is premium function _getMetadataForTokenId(uint256 tokenId) internal view returns (uint256 neighborhoodId, bool premium) { @@ -71,16 +69,16 @@ abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { premium = (metadata & PREMIUM_MASK) != 0; } - /// @notice set the packed metadata for 32 lands at once - /// @param tokenId the base token id floor 32 - /// @param metadata the packed metadata for 32 lands + /// @notice set the packed metadata for LANDS_PER_WORD lands at once + /// @param tokenId the base token id floor LANDS_PER_WORD + /// @param metadata the packed metadata for LANDS_PER_WORD lands function _setMetadata(uint256 tokenId, uint256 metadata) internal { LandMetadataStorage storage $ = _getLandMetadataStorage(); $._metadata[_getKey(tokenId)] = metadata; } - /// @notice return the packed metadata for 32 lands at once - /// @param tokenId the base token id floor 32 + /// @notice return the packed metadata for LANDS_PER_WORD lands at once + /// @param tokenId the base token id floor LANDS_PER_WORD function _getMetadata(uint256 tokenId) internal view returns (uint256) { LandMetadataStorage storage $ = _getLandMetadataStorage(); return $._metadata[_getKey(tokenId)]; @@ -110,7 +108,7 @@ abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { return (tokenId % LANDS_PER_WORD) * BITS_PER_LAND; } - /// @notice return the tokenId floor 32 + /// @notice return the tokenId floor LANDS_PER_WORD /// @param tokenId the token id function _getKey(uint256 tokenId) internal pure returns (uint256) { return LANDS_PER_WORD * (tokenId / LANDS_PER_WORD); @@ -123,7 +121,7 @@ abstract contract LandMetadataBase is AccessControlEnumerableUpgradeable { if (neighborhoodId == 0) { revert InvalidNeighborhoodId(neighborhoodId); } - // NEIGHBORHOOD_MASK (32767) is left out to use as escape char if needed. + // the last id: NEIGHBORHOOD_MASK is left out to use as escape char if needed in the future. if (neighborhoodId >= NEIGHBORHOOD_MASK) { revert InvalidNeighborhoodId(neighborhoodId); } diff --git a/packages/land/test/common/ERC721.behavior.ts b/packages/land/test/common/ERC721.behavior.ts index b431f1dcae..a306b5b38e 100644 --- a/packages/land/test/common/ERC721.behavior.ts +++ b/packages/land/test/common/ERC721.behavior.ts @@ -857,6 +857,25 @@ export function shouldCheckForERC721(setupLand, Contract: string) { expect(await LandAsOwner.supportsInterface('0x2a55205a')).to.be.true; }); + it('claim to ILandToken interface combined with IQuad interface', async function () { + const {LandAsOwner} = await loadFixture(setupLand); + expect(await LandAsOwner.supportsInterface('0xe906a607')).to.be.true; + }); + + it('claim to ILandToken interface', async function () { + const {LandAsOwner} = await loadFixture(setupLand); + expect(await LandAsOwner.supportsInterface('0x3b18763a')).to.be.true; + }); + it('claim to IQuad interface', async function () { + const {LandAsOwner} = await loadFixture(setupLand); + expect(await LandAsOwner.supportsInterface('0xd21ed03d')).to.be.true; + }); + + it('claim to ILandMetadataRegistry interface', async function () { + const {LandAsOwner} = await loadFixture(setupLand); + expect(await LandAsOwner.supportsInterface('0x519cd8d9')).to.be.true; + }); + it('does not claim to support random interface', async function () { const {LandAsOwner} = await loadFixture(setupLand); expect(await LandAsOwner.supportsInterface('0x88888888')).to.be.false;