Skip to content

Commit

Permalink
fix: apply small changes after the audit
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
Andres Adjimann committed Jun 27, 2024
1 parent 3911872 commit 4d9654b
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 43 deletions.
6 changes: 3 additions & 3 deletions packages/land/contracts/LandMetadataRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 8 additions & 1 deletion packages/land/contracts/common/LandBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
}
}
2 changes: 1 addition & 1 deletion packages/land/contracts/common/WithMetadataRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 3 additions & 26 deletions packages/land/contracts/interfaces/ILandToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,13 @@

pragma solidity ^0.8.0;

import {IQuad} from "./IQuad.sol";

/// @title ILandToken
/// @author The Sandbox
/// @custom:security-contact [email protected]
/// @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
Expand Down
34 changes: 34 additions & 0 deletions packages/land/contracts/interfaces/IQuad.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

/// @title IQuad
/// @author The Sandbox
/// @custom:security-contact [email protected]
/// @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;
}
22 changes: 10 additions & 12 deletions packages/land/contracts/registry/LandMetadataBase.sol
Original file line number Diff line number Diff line change
@@ -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 [email protected]
/// @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);
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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)];
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down
19 changes: 19 additions & 0 deletions packages/land/test/common/ERC721.behavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

1 comment on commit 4d9654b

@github-actions
Copy link

Choose a reason for hiding this comment

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

Coverage for this commit

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
packages/land/contracts
   Land.sol100%100%100%100%
   LandMetadataRegistry.sol100%100%100%100%
   PolygonLand.sol100%100%100%100%
packages/land/contracts/common
   ERC721BaseToken.sol100%100%100%100%
   LandBase.sol100%100%100%100%
   LandBaseToken.sol100%100%100%100%
   OperatorFiltererUpgradeable.sol100%100%100%100%
   WithAdmin.sol100%100%100%100%
   WithMetadataRegistry.sol100%100%100%100%
   WithOwner.sol100%100%100%100%
   WithRoyalties.sol100%100%100%100%
   WithSuperOperators.sol100%100%100%100%
packages/land/contracts/interfaces
   IERC173.sol100%100%100%100%
   IERC721BatchOps.sol100%100%100%100%
   IERC721MandatoryTokenReceiver.sol100%100%100%100%
   IErrors.sol100%100%100%100%
   ILandMetadataRegistry.sol100%100%100%100%
   ILandToken.sol100%100%100%100%
   IOperatorFilterRegistry.sol100%100%100%100%
   IQuad.sol100%100%100%100%
packages/land/contracts/mainnet
   LandStorageMixin.sol100%100%100%100%
packages/land/contracts/polygon
   ERC2771Handler.sol100%100%100%100%
   PolygonLandStorageMixin.sol100%100%100%100%
packages/land/contracts/registry
   LandMetadataBase.sol100%100%100%100%

Please sign in to comment.