Decentraland Audit Report

Our audit report on components of the Marketplace and Districts features.

Summary

The Decentraland team asked us to audit their MarketplaceBurner contract at commit d5ff5e089e3204b0138b440dd77cae573a3108f9 and their PassThrough contract at commit 9d83d9edd6c7b22cbf63c428ae0ab39a7d618557.

No vulnerabilities were found. Additional recommendations were made around the implementation.

Full Report

Related contracts and assumptions

The following assumptions were made during the audit:

Estate registry contract

• PassThrough#estateRegistry points to the ZeppelinOS-based upgradable smart contract located at 0x959e104e1a4db6317fa58f8295f586e1a978c297, with its implementation contract located at 0xe34fcd12115bfe2fb632cd63fe7be5472b54173f.
• PassThrough.sol#target will point to that same contract and that operators will rarely change it in order to fix some mistake (e.g. send MANA or LAND to the PassThrough contract).

Implementation’s source code: EstateRegistry.sol

Land registry contract

• EstateRegistry#registry points to the ZeppelinOS-based upgradable smart contract located at 0xf87e31492faf9a91b02ee0deaad50d51d56d5d4d, with its implementation contract located at 0xf8550b0511486af9a75c63d85a01c919535f76db.

Implementation’s source code: LANDRegistry.sol

MANA contract

• MarketplaceBurner#mana points to the smart contract located at 0x0f5d2fb29fb7d3cfee444a200298f468908cc942.

Source code: MANAToken.sol

Marketplace contract

• MarketplaceBurner#marketplace points to the ZeppelinOS-based upgradable smart contract located at 0x8e5660b4ab70168b5a6feea0e0315cb49c8cd539, with its implementation contract located at 0x19a8ed4860007a66805782ed7e0bed4e44fc6717.

Implementation’s source code: Marketplace.sol

Contracts ownership

• PassThrough contracts will be owned by Decentraland, and their operators will be external parties.
• MarketplaceBurner will own the Marketplace and will be owned by Decentraland.

Audit results

Critical severity

No critical severity issues were found.

High severity

No high severity issues were found.

Medium severity

[DCL-M01] Possible accidents on Estate registry upgrades

If the Estates registry contract is updated with a new function for transferring LAND, such function must be blacklisted in advance in every instance of PassThrough. A failure to do so can result in an Estate being disassembled.

Our recommendation is to invert the PassThrough contract logic, implementing it as a whitelist instead of a blacklist. A whitelist approach has a much smaller impact if a function isn’t listed, just temporarily degrading the user experience.

One thing to note is that the whitelist should only be applied when the target is the estateRegistry. Otherwise, the users won’t be able to change the target to remedy any incident on their own, as most/all functions they need to call won’t be whitelisted.

Update: Decentraland informed that this alternative was already considered, and they will take care of updating all PassThrough instances as part of the LAND upgrade process.

Low severity

[DCL-L01] PassThrough payable target functions can’t be called with ETH

There’s no way to call a target’s function with ETH, as PassThrough’s fallback function is not payable, and its assembly call invocation always set the value as 0. In order to fix this, the fallback function has to be payable, and msg.value should be passed to call.

This may have an impact for users that are changing the contract’s target to fix some mistake. It’s hard to predict if this scenario is feasible or not, but the fixes needed to avoid it are really straight forward and we recommend implementing them.

Update: This issue has been fixed in PR #3.

[DCL-L02] Missing checks in PassThrough constructor

The PassThrough constructor doesn’t validate the values of the _estateRegistry and _operator addresses. While this could lead to accidentally deploying it with wrong values, its impact would be very low, as the contract can be redeployed.

Update: This issue has been fixed in PR #3.

[DCL-L03] Missing checks in MarketplaceBurner constructor

The MarketplaceBurner constructor doesn’t validate the values of the MANA and Marketplace addresses. While this could lead to accidentally deploying it with wrong values, its impact would be very low, as the contract can be redeployed.

Our recommendation is to provide a deployment script that uses the suitable addresses when running against mainnet. This makes the deployment auditable and testable.

[DCL-L04] Disabled functionality

The following Marketplace functions are not exposed by the MarketplaceBurner:

• setLegacyNFTAddress
• setPublicationFee
• cancelOrder

The impact of this depends on how often these functions are called and should be assessed by Decentraland. Our opinion is that only cancelOrder and setPublicationFee are going to be needed.

Comments

Multiple unit tests don’t check that the expected results are met

There are multiple tests that call a PassThrough contract without checking its result (e.g. this test). This can hide possible bugs, as every non-blacklisted call will be redirected to the target, but not necessarily to the right function.

Missing functions in the end to end tests

The following functions aren’t tested in the end to end tests:

• safeTransferFrom(address,address,uint256,bytes)
• safeTransferManyFrom(address,address,uint256[],bytes)

No test of failures in the target contract

The test suite doesn’t include cases where the call to the target contract reverts. This is especially important to be sure that error messages are correctly forwarded.

Update: This issue has been fixed in PR #3 and PR #4.

Code coverage couldn’t be computed

As part of the audit process, we tried to analyze the code coverage of the contract, with no luck in computing it.

The standard tool for doing so, solidity-coverage, failed to work with this contract due to its parser being outdated. After manually changing the source code to work around this situation, other errors arose that couldn’t be solved.

Despite not being able to quantify the coverage and with exception of the comments stated above, the contract is very well tested.

Missing check in transferMarketplaceOwnership

This function doesn’t check if the new target is address(0), which is considered a good practice but Ownable#transferOwnership does it, so address(0) can’t be accidentally used.

Missing tests

No automated tests were provided along the MarketplaceBurner contract. We recognize that there’s little value to be added by unit tests in this case, but integration tests would be valuable.

We recommend automating the deployment of a test environment for every, or at least most, Decentraland smart contracts, as there are multiple interconnected parts. Not having such script leads to repeating work or avoiding tests altogether.

Conclusion

No security issues were found. Some changes were proposed to reduce the risk of potential future human mistakes.