The Sandbox security record

In the contract `PolygonLandV1`, the role `_admin` has authority over the following functions: - `setPolygonLandTunnel()`: Modify the `PolygonLandTunnel` address; - `setTrustedForwarder()`: Modify the Trusted Forwarder for the Meta Transactions. In addition, the role `polygonLandTunnel` has authority over the following function: - `mint()`: mints quads to an address. The contract `PolygonLandV1` inherits the contract `PolygonLandBaseToken`, where the role `superOperator` has authority over the following functions: - `batchTransferQuad()`: Transfer any user's quads to an address; - `transferQuad()`: Transfer any user's quads to an address. The contract `PolygonLandBaseToken` inherits the contract `ERC721BaseToken`, where `_admin` has authority over the following functions: - `setSuperOperator()`: Give or remove the `superOperator` role to or from an address; - `changeAdmin()`: Change the address of the role `_admin`. In addition, the `superOperator` role has authority over the following functions: - `approve()`: Decide the allowance of any token; - `approveFor()`: Decide the allowance of any token; - `transferFrom()`: Transfer any user's tokens to an address; - `safeTransferFrom()`: Transfer any user's tokens to an address; - `batchTransferFrom()`: Transfer several of a user's tokens to an address; - `safeBatchTransferFrom()`: Transfer several of a user's tokens to an address; - `setApprovalForAllFor()`: Set the approval for an address to manage all of a user's tokens; - `burnFrom()`: Burn any user's tokens. In the contract `LandTunnel`, the role `_owner` has authority over the following functions: - `setTrustedForwarder()`: Modify the Trusted Forwarder for the Meta Transactions; - `pause()`: Pause the contract to disable transfers from L1 (Ethereum) to L2 (Polygon); - `unpause()`: Unpause the contract, reenabling transfers from L1 (Ethereum) to L2 (Polygon). In the contract `PolygonLandTunnel`, the role `_owner` has authority over the following functions: - `setMaxLimitOnL1()`: Define the maximum amount of gas to spend for `batchTransferQuadToL1()` function; - `setMaxAllowedQuads()`: Define the maximum amount of quads to transfer for `batchTransferQuadToL1()` function; - `setLimit()`: Define the maximum amount of gas to spend on transfers for one quad type; - `setupLimits()`: Define the maximum amount of gas to spend on transfers for each quad type; - `setTrustedForwarder()`: Modify the Trusted Forwarder for the Meta Transactions; - `pause()`: Pause the contract, preventing transfers from L2 (Polygon) to L1 (Ethereum); - `unpause()`: Unpause the contract, reenabling transfers from L2 (Polygon) to L1 (Ethereum). Any compromise to the aforementioned privileged accounts may allow a hacker to take advantage of this authority and manipulate the reward system.

The function “_computeValue” is meant to calculate a random value for assigning the value per gem ID. The result of its computation is stored in a variable named “randomValue”.

The report gives no description here.

Sandbox has implemented Land token protocols on both L1 (Ethereum) and L2 (Polygon). The burning logic of the two protocols is different.\ On L2, the `_burn()` function sets the 160st bit of the token's owner record to be 1 (as an identification for burnt tokens) yet maintains the previous owner's address. ```solidity= //Burning Logic on L2 function _burn( address from, address owner, uint256 id ) internal { require(from == owner, "NOT_OWNER"); uint256 storageId = _storageId(id); _owners[storageId] = (_owners[storageId] & NOT_OPERATOR_FLAG) | BURNED_FLAG; // record as non owner but keep track of last owner _numNFTPerAddress[from]--; emit Transfer(from, address(0), id); } ``` On L1, the `_burn()` function sets the token's owner record as 2**160 and removes the previous owner's address. ```solidity= //Burning Logic on L1 function _burn(address from, address owner, uint256 id) internal { require(from == owner, "not owner"); _owners[id] = 2**160; // cannot mint it again _numNFTPerAddress[from]--; emit Transfer(from, address(0), id); } ``` If tokens on L1 and L2 are designed to be equal, it is recommended to maintain the same logic to avoid unexpected consequences resulting from protocol incompatibility.

When the contract tries to mint a quad via `_mintQuad()`, there is a check to ensure that no quads containing the quad to mint and no quads (or LANDS) within the quad to mint have already been minted by calling the function `exists()`. This leads to a possible denial-of-service attack where the attacker mints 1x1 LANDS at specific locations to prevent the minting of larger quads. For example, out of the possible 166,464 LAND placements, only 289 LANDS need to be minted to prevent 24x24 quads from occurring. **Proof of Concept**\ The following test shows that if 289 1x1 Land is minted, no one can mint 24x24 land anymore. ```typescript it('Mint 289 land to prevent others mint 24x24', async function () { const {landOwners} = await setupTest(); const bytes = '0x3333'; let totalMint = 0; for (let x = 0; x < GRID_SIZE; x = x + 24) { for (let y = 0; y < GRID_SIZE; y = y + 24 ){ await waitFor( landOwners[0].MockLandWithMint.mintQuad( landOwners[0].address, 1, x, y, bytes )); //console.log('Minted a land on (%d,%d)', x , y) totalMint = totalMint + 1; } } console.log('Step1: Mint lands in each 24x24 land. In total %d 1x1 land is minted', totalMint); let totalTry = 0; for (let x = 0; x < GRID_SIZE; x = x + 24) { for (let y = 0; y < GRID_SIZE; y = y + 24) { await expect( landOwners[0].MockLandWithMint.mintQuad( landOwners[0].address, 24, x, y, bytes ) ).to.be.revertedWith('Already minted'); totalTry = totalTry + 1; } } console.log('Step2: Cannot Mint 24x24 land Anymore. In total %d times tried to mint 24x24 land', totalTry) }); ``` The test case passed, meaning 24x24 land can no longer be minted ```typescript Step1: Mint lands in each 24x24 land. In total 289 1x1 land is minted Step2: Cannot Mint 24x24 land Anymore. In total 289 times tried to mint 24x24 land ✓ Mint 289 land to prevent others mint 24x24 (18107ms) ```

The contract is serving as the underlying entity to interact with third-party **fx-portal** protocols. The **fx-portal** protocol performs the cross-chain functionality to exchange messages between Ethereum and Polygon. Land Bridge interacts with the following contracts: - [FxBaseChildTunnel](https://github.com/fx-portal/contracts/blob/main/contracts/tunnel/FxBaseChildTunnel.sol) - [FxBaseRootTunnel](https://github.com/fx-portal/contracts/blob/main/contracts/tunnel/FxBaseRootTunnel.sol) The scope of the audit treats 3rd party entities as black boxes and assumes their functional correctness. However, in the real world, 3rd parties can be compromised, which may lead to lost or stolen assets.

In the current implementation, when the `LandSwap` contract is not in the pause condition, the `burn()` function allows everyone to burn `oldLand` tokens with given `ids`. The `LandSwap` contract receives `oldLand` tokens during the `swap()` invocation and those tokens are supposed to be burnt immediately. The concern is if those `_oldland` tokens are still valuable to some extent (f.e., causing market fluctuation), a user might call this function at a certain time for his own profit.

In `LandSwap.sol`, no checks are performed on input parameters to ensure that those are ones that are valid. In the `LandSwap` contract, in the `initialize()` function : - `admin` and `trustedForwarder` addresses should be non-zero addresses; - `oldLand` should not be the same address as `newLand`; - `admin` should not be the same address as `trustForwarder`.

The contract serves as the underlying entity to interact with the following third-party protocols. - `_oldLand` - `_newLand` - `trustedForwarder` The scope of the audit treats third-party entities as black boxes and assumes their functional correctness. However, in the real world, third parties can be compromised, which may lead to lost or stolen assets. For instance, if the `trustedForwarder` is initialized to a malicious address, it could cause unexpected errors during the land-swapping process.

The implementation of `_metaTransactionContracts[msg.sender]` is unknown. In the case that `metaTransaction` contracts are public and propose a `call()` feature, users could call a `metaTransaction` contract, having their call being forwarded to `ERC721BaseToken`. In this scenario, `msg.sender` received by `ERC721BaseToken` would be the address of `metaTransaction`, which would verify the condition `require(_metaTransactionContracts[msg.sender]`. If the check can be bypassed through this way, this would be problematic in the functions using this check: - `ERC721BaseToken`.`approveFor()`; - `ERC721BaseToken`.`_checkTransfer()`; - `ERC721BaseToken`.`_batchTransferFrom()`; - `ERC721BaseToken`.`setApprovalForAllFor()`; - `ERC721BaseToken`.`burnFrom()`. For example, ```solidity=301 function setApprovalForAllFor( address sender, address operator, bool approved ) external { require(sender != address(0), "Invalid sender address"); require( msg.sender == sender || _metaTransactionContracts[msg.sender] || _superOperators[msg.sender], "not authorized to approve for all" ); _setApprovalForAll(sender, operator, approved); } ``` The above function will successfully triggered if the caller is from meta transaction contract (i.e., `_metaTransactionContracts[msg.sender]` is `true`).