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:
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
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
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.
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:
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.
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:
This dependency is not being used and can be removed to reduce the code size of the smart contract.
This dependency is not being used and can be removed to reduce the code size of the smart contract.
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.
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.
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.
Technically this require statement is redundant, as the correct approval state is being checked subsequently in the transferFrom call in line 127, see:
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.
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.
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.
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.
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.
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.
As of the date of publication, the information provided in this report reflects the presently held understanding of the auditor’s knowledge of security patterns as they relate to the client’s contract(s), assuming that blockchain technologies, in particular, will continue to undergo frequent and ongoing development and therefore introduce unknown technical risks and flaws. The scope of the audit presented here is limited to the issues identified in the preliminary section and discussed in more detail in subsequent sections. The audit report does not address or provide opinions on any security aspects of the Solidity compiler, the tools used in the development of the contracts or the blockchain technologies themselves, or any issues not specifically addressed in this audit report.
The audit report makes no statements or warranties about the utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, the legal framework for the business model, or any other statements about the suitability of the contracts for a particular purpose, or their bug-free status.
To the full extent permissible by applicable law, the auditors disclaim all warranties, express or implied. The information in this report is provided “as is” without warranty, representation, or guarantee of any kind, including the accuracy of the information provided. The auditors hereby disclaim, and each client or user of this audit report hereby waives, releases and holds all auditors harmless from, any and all liability, damage, expense, or harm (actual, threatened, or claimed) from such use.
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.
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.
