The AdEx network team asked us to audit the contracts they have build to run their decentralized Ad Exchange contracts.
Audited code was located at https://github.com/AdExBlockchain/adex-core
The commit hash for the audited code:
e724a6dc3499b8fc64ed692a1ff3c9b03458faf4
contracts/ADXExchange.sol
contracts/ADXRegistry.sol
This audit was performed by Modular, Inc to help AdEx identify potential security vulnerabilities in their Ad Registry and Ad Exchange contracts. Two contracts and their accompanying tests were audited.
Overall, these crowd sale contracts were written fairly well, using trusted methods and safe coding styles. Using the Zeppelin Ownable, Drainable, ERC20, and SafeMath contracts was a smart choice, cutting down on potential security issues.
No critical issues were discovered with these contracts. Some minor issues that affect usability and protect from misuse were found and amended. There are also minor styling and documentation issue recommendations which can be subjective.
Critical issues can be defined as those items that would cause the logic to not function as advertised. For example, logic that could be exploited for an independent party’s gain due to lack of authorization checks, code that would cause funds to be trapped due to non-bounds checking, or outright broken logic.
No critical issues were discovered in this audit.
Major issues can be defined as those items that would allow any party to have more leverage than anticipated, but not as serious as critical issues. For example, logic that could be exploited unexpectedly by the owner or would allow otherwise trusted participants to have advantage.
ADXExchange.sol
Description
Lines 135, 136, 192, and 193: Calls generic getter functions from the ADX Registry contract to serve dual purpose of retrieval and validation
Suggestion
Create explicit functions that are purposeful toward this goal. This does not appear to be meaningful on initial release, however, future code updates may cause unforeseen breaking changes without explicit functions that tie information ADX Exchange needs from ADX Registry
ADXRegistry.sol
Description
There is a consistent use of `var` and `uint` which does not allow a clear definition for variables and types.
Suggestion
Implicit type definitions leave code reviewers to make assumptions. Whether it be for users, internal development updates, or auditors, explicitly defining types rather than relying on `var` or `uint` allows for easier code reviews in the future. Reviewers are not left to make any assumptions and can make clear connections.
Description
Functions and their arguments are not consistently documented.
Suggestion
It is good coding practice to have a consistent method for documenting functions and variables. It is up to the team to choose what works best for them, but the Ethereum Natspec format is a good place to start.
Description
Arguments to functions are not put in a consistent order.
Suggestion
It is good coding practice for similar functions to always order their arguments in the same relative order. For example, in line 60 for register(), the name argument is the first argument, whereas in line 86 for registerItem, name is the fourth argument. Aligning the order of arguments may make it easier for users to interact with the contract without making mistakes.
Description
Line 22 and 23: counts and items names are not descriptive.
Suggestion
It is good coding practice to name variables in a contract in an easy to understand way. Adding a more descriptive name for counts and items would make for better code readability. Something like itemCounts or itemsByType would suffice.
Description
Some member variables in structs are not completely necessary, such as name and meta.
Suggestion
It is common practice to only include information, functionality, and variables in an Ethereum smart contract that is strictly necessary for the contract to function. Unless these variables are needed for the contract to function, removing them could be a good choice to reduce Gas costs, provide simpler interfaces to users, and reduce the amount of variables that need to be accounted for in the code. This extra information can be kept off chain with a reference to the on-chain ID.
Description
Line 60: `register()` function has no check for `_sig`. Same for `_type` in `registerItem()` function.
Suggestion
Consider whether or not this is purposeful and if `_sig` should be required and/or `_type < enum.length`
Description
Line 86: `registerItem()` function does not have `external` scope definition as was put for `register()`.
Suggestion
For both contracts, the `register()` function was the only non-getter function that explicitly stated the scope. Code should be consistent and scope definitions also show purpose. Consider stating the proper level of scope for all functions.
ADXExchange.sol
Description
Line 118: Arguments to constructor are not checked for validity
Suggestion
All potential inputs and mistakes should be considered and checked for, even in the constructor. Check that the two arguments aren’t 0.
Description
Line 98: `onlyBidAceptee()` not spelled correctly nor does it accurately represent the relationship
Suggestion
Change to `onlyBidAccepter()` to correct this
In conclusion, these contracts were written fairly well and there aren’t any critical issues with security. Testing could be expanded some to have a more objective guarantee of quality, but is not absolutely necessary. With the suggested changes, the contract should behave consistently and be easier for participants to understand and use correctly.
Modular provides smart contract services and security reviews for software intended to be deployed on EVM enabled networks in addition to working on open source projects in the Ethereum community. We also test, document, and deploy reusable code onto the blockchain and improve both security and usability through our code libraries contained at https://github.com/Modular-Network/ethereum-libraries. Finally, we also enjoy educating non-profits, schools, and other community members about the application of blockchain technology.
For further information: majoolr.io, contact@majoolr.io
