Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Decentraland : Forwarder - Security_Review.md

Decentraland : Forwarder - Security Review

Date: Ariel Barmat - 06/2021

Introduction

The Decentraland Team requested the review of the contract deployed on address https://etherscan.io/address/0x34ed9e73930290cec0cfe601809255ec1313ea18

The contract is called Forwarder.sol, the SHA-256 of the flattened code as copied from Etherscan is 981c88c668c201381b3205c4bd75c4584a38e66b16a130f80a89de44a14b43a0.

The review was performed directly on the deployed code, I assume the Decentraland team created a dedicated test suite on the project's contract repo.

Summary

No critical nor high vulnerabilities were found on the contract code. The code is well written and well documented.

Background

The Decentraland Foundation needs to manage public Estates identified as "roads" that are under the control of an entity called the DAO-Agent. Estates are implemented as NFTs using an upgradeable contract that follows the Transparent Proxy pattern. The issue arises because the DAO-Agent is both the ProxyOwner of the EstateRegistry and the Owner of some Estates. Under a Transparent Proxy, the DAO-Agent is not able to access the NFT functions, only the proxy admin ones.

Decentraland is introducing the Forwarder contract to be the co-owner of estates along with the DAO-Agent, effectively allowing the Forwarder to call the Estate contract implementation functions. The Owner of the Forwarder MUST be the DAO-Agent for this setup to work and ensure the sanity of the governance rules.

Current Setup

Planned Setup

Audit

The contract is compiled using Solidity 0.8.4 with optimizations disabled. The code is well written and has good documentation.

Roles and their security assumptions

  • Owner: The owner is set when the contract is deployed. Additionally it can be changed by the current owner by using the transferOwnership() call.

Low Severity

[L1] Add a check to check if target address is a contract

As stated in the Background section, the Forwarder contract is going to be used to perform calls to other contracts. For that reason it might be good to explicitly revert if the target address is an EOA.

Considering using OpenZeppelin's Address Library. It implements a function call that performs the contract validation and additionally bubbles up the revert reason https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4d0f8c1da8654a478f046ea7cf83d2166e1025af/contracts/utils/Address.sol#L122

Notes

  • N1 - The contract has no limitation on the assets it can manage as it can do any arbitratry contract call. Evaluate if that is an expected behaviour or better to validate that the target is only the EstateRegistry contract.

Operational Remarks

  • The contract does not impose any limit on the type or scope of assets it can manage. The main area of risk is related to the permissions given to the Forwarder. The team needs to ensure that the proper setOperator permissions are given to this contract to avoid it managing unexpected assets.
  • The team should ensure that the right owner is configured on deployment time.
@yemel

This comment has been minimized.

Copy link

@yemel yemel commented Jun 22, 2021

Thanks Ariel for performing this audit.
One comment, the image link of the "Planned Setup" it's broken from me.
The rest LGTM.

@abarmat

This comment has been minimized.

Copy link
Owner Author

@abarmat abarmat commented Jun 22, 2021

@yemel I hosted the files on IPFS that one might be unpinned, I'll check it up 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment