Ember Sword NFT & Sale

May 21, 2021

1. Preface

The team of Bright Star contracted byterocket to conduct a smart contract audit of the NFT and Land Sale smart contracts for Ember Sword. Ember Sword is a novel Massively Multiplayer Online Role-playing Game, where users can purchase in-game land plots as NFTs on the blockchain in order to verifiably own and trade land plots within the game.

The contracts functionality is split between the NFT part and the Sale part within different contracts. The contracts each feature a proxy, making them upgradeable.

The team of byterocket reviewed and audited the above smart contracts in the course of this audit. We started on the 4th of May and finished on the 10th of May. An updated version of the smart contract was ultimately audited again on the 20th of May.

The audit included the following services:

  • Manual Multi-Pass Code Review
  • Automated Code Review
  • Formal Report

byterocket gained access to the code via a privately shared link, since the code was not public at the time of the audit. The shasum hashes of the different smart contracts at the time of the audit are listed below:

SaleImplementation.sol: c709dbc1ff88e04df3988b413264ea5733f9fe67
SaleProxy.sol: a0a8b466f4668ef85952b1460cc6178db7a3c96a
NFTImplementation.sol: 05c09f478a1ecf6c8dd7e2c7ae89227739c6abdd
NFTProxy.sol: 35cc1a4e8a8a12398a1ae99f4cd71f8efef722b8

Update on the 21st of May 2021

The developers have updated their contracts according to our recommendations. The shasum hashes of the different smart contracts at the time of this update are listed below:

SaleImplementation.sol: f18ed848b86d813ae2013a3f7539d844ebbc592b
SaleProxy.sol: da9c37ec721c9ca9ed0638f54ba97531065047ab
NFTImplementation.sol: e84eba7b4318bdafd06ef56e8b27a43d5a96ca50
NFTProxy.sol: ce9f1f7c6ebfe2d953aab5d54ab96645299e8afc

2. Manual Code Review

We conducted a manual multi-pass code review of the smart contracts mentioned in section (1). Three different auditors went through the smart contract independently and compared their results in multiple concluding discussions.

These contracts are written according to the latest standards used within the Ethereum community and the Solidity community’s best practices. The naming of variables is very logical and understandable, which results in the contract being easy to understand. The code is very well documented and up to the latest standards.

On the code level, we found two low severity and seven no severity bugs or flaws. A further check with multiple automated reviewing tools (MythX, Slither, Manticore, and different fuzzing tools) did not find any additional bugs besides some common false positives.
The developers have since updated their smart contracts according to our recommendations, which fixed the vulnerabilities and bugs that we found. Consequently we have found no bugs or flaws in the updated code.

2.1. Bugs & Vulnerabilities

A) NFTImplementation.sol - Line 16  [LOW SEVERITY] [FIXED]
import "@openzeppelin/contracts/utils/Counters.sol";

The smart contract is the implementation of an upgradeable proxy system. However, some of the imports are not using upgradeable dependencies, like the one above. For most of them this is not an issue, but since the Counters library is storing data, the upgradeable version should be used like this:

@openzeppelin/contracts-upgradeable/utils/CountersUpgradeable.sol

The upgradeable version uses the upgradeable SafeMath version, making it safe to be upgraded. This switch ensures that no unforeseen consequences arise due to an upgrade later on, since it is being used to store the token IDs.

B) SaleImplementation.sol - Line 15 + 16  [LOW SEVERITY] [FIXED]
import "@openzeppelin/contracts/math/SafeMath.sol";
import "@openzeppelin/contracts/utils/Address.sol";

The smart contract is the implementation of an upgradeable proxy system. However, some of the imports are not using upgradeable dependencies, like the ones above. This does not necessarily mean that problems will arise when upgrading the contracts later on, however, we still advise to use the upgradeable-safe versions, like:

@openzeppelin/contracts-upgradeable/math/SafeMathUpgradeable.sol
@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol

2.2 Other Findings

A) NFTImplementation.sol - Line 17  [NO SEVERITY] [FIXED]
import "@openzeppelin/contracts/utils/EnumerableSet.sol";

This dependency is not being used and can be removed to reduce the code size of the smart contract.


B) NFTImplementation.sol - Line 18  [NO SEVERITY] [FIXED]
import "@openzeppelin/contracts/utils/Address.sol";

This dependency is not being used and can be removed to reduce the code size of the smart contract.


C) NFTImplementation.sol - Line 88 + 112  [NO SEVERITY] [FIXED]

The two functions mintProperty() and getTokenIdByCoordinates() are not being used within the contract and can be declared as external instead of public to save some gas.


D) SaleImplementation.sol - Line 40-60  [NO SEVERITY]

None of the events have any indexed properties, which sometimes makes it hard for some frontends to access them quickly, depending on the used technique. If this is irrelevant to the developer(s), this issue can be disregarded, e.g. when using the Graph.

E) SaleProxy.sol & NFTProxy.sol - Line 43  [NO SEVERITY] [FIXED]

function upgradeTo(address _logic, bytes memory _data) public onlyOwner

This function is not being used within the contract and can be declared as external instead of public to save some gas.


F) SaleImplementation.sol - Line 120-123  [NO SEVERITY]
require(
 ERC721(nftAddress).getApproved(nftId) == address(this),
 "Grant NFT approval to Sale Contract"
);

Technically this require statement is redundant, as the correct approval state is being checked subsequently in the transferFrom call in line 127, see:

function transferFrom(address from, address to, uint256 tokenId)  
         public virtual override  
  require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer
          caller is not owner nor approved");
  _transfer(from, to, tokenId);
}

We understand that there might be certain reasons why an approval is checked within an own require statement for error message response, but we still advise to reconsider this.

Update on the 21st of May

The developers stated that this is done on purpose in order to provide a meaningful error message early on instead of one that is hidden in the ERC contract.

3. Protocol/Logic Review

Part of our audits are also analyses of the protocol and its logic. A team of three auditors went through the implementation and documentation of the implemented protocol.

We mainly focused on the SaleImplementation contract when looking at the protocol, since this contract mainly deals with any economically incentivized actions like sales and buys.

In our discussion we were not able to find any weaknesses within the implementation of the protocol. None of our function calls (even when we did randomized fuzzing tests) were able to produce any unforeseen outcomes and skew the results in any way. The overall process flow was valid at all times.

Since the sale process is quite basic with one seller and one direct buyer, we couldn’t find any game-theoretic problems as well.
Thus, We were not able to discover any problems in the protocol implemented in the smart contract.

4. Testnet Deployment

As per our testing strategy, we deploy audited smart contracts (if requested by the client) onto a testnet to verify the implementation’s functionality. We usually deploy simple smart contracts to a local Ganache instance, which is sufficient for most cases. In this case. we wanted to ensure no Ganache-related coverups of the contracts’ misbehavior. We created two testnets: a geth-based one and a parity/openethereum-based one. All of our tests have been executed on both testnets without any difference in the results. We were able to use the contracts as intended and could not maliciously game the protocol in practice.

We used fuzzing tools that generate random and semi-random inputs and interact with the contracts, trying to produce any unforeseen states within the contracts, especially the treasury contract. Throughout our tests, we were not able to create or observe any issues or problems. The contract behaved as expected and reverted correctly, given wrong inputs. No unforeseen states occurred during our fuzz-tests.

4.1. Unit Tests and Coverage

When looking at how the contracts behave in practice, we also take a look at the provided unit tests. The overall quality of the repository and its structure is very high, which does also include well-written unit tests with a good coverage.

All of the current best-practice checks have been implemented, including for checks whether certain events have been emitted. Especially the functions of the sale contract have been tested very well and with a very outstanding coverage.
In our tests none of the provided unit tests failed. All in all, we are very satisfied with the provided unit tests, their quality and the overall test coverage. We were not able to find any issues with the provided tests.

5. Summary

During our code review (which was done manually and automated), we found two low severity and seven no severity bugs or flaws. Our automated systems and review tools also did not find any additional ones. The developers have since updated their smart contracts according to our recommendations, which fixed the vulnerabilities and bugs that we found. Consequently we have found no bugs or flaws in the updated code.

The protocol review and analysis did neither uncover any game-theoretical nature problems nor any other functions prone to abuse.

During our multiple deployments to various local testnets, we haven’t been able to find any additional problems or unforeseen issues. The provided unit tests worked flawlessly and we were also not able to find any issues here.
In general, we are delighted with the overall quality of the code, its tests and documentation.

Stored on IPFS

We store our public audit reports on IPFS; a peer-to-peer network called the "Inter Planetary File System". This allows us to store our reports in a distributed network instead of just a single server, so even if our website is down, every report is still available.

Learn more about IPFS
Signed On-Chain

The IPFS Hash, a unique identifier of the report, is signed on-chain by both the client and us to prove that both sides have approved this audit report. This signing mechanism allows users to verify that neither side has faked or tampered with the audit.

Check the Signatures