abarmat/Decentraland_Forwarder_Security_Review.md Secret

## Decentraland_Forwarder_Security_Review.md

      
    Raw
  

              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.