New Alchemy Security Audit: Findings
Jibrel engaged New Alchemy to conduct a full security audit of Jibrel Smart Contracts —Full results published here
We’re pleased to announce that we have recently completed our Security Audit. The audit was undertaken by the world renowned white hat hacker group, New Alchemy (the team that first identified the DAO Hack exploit).
We are especially pleased to report that no critical or major security flaws were identified, but numerous gas / code optimization areas were found. All security gaps, optimization areas and flaws, highlighted by New Alchemy, have subsequently been addressed by the Jibrel team.
Executive Summary
No critical errors were found during this audit. Significant findings such as the short address and double spend attacks are weaknesses identified in the ERC20 standard and EVM behavior themselves and must be considered for every ERC20 implementation.
Introduction
On the 27th of July, 2017, the Jibrel team engaged New Alchemy to audit their smart contract implementations of Crypto Depository Receipts (CryDRs) and Jibrel Network Tokens (JNTs). The audit was technical in nature with the purpose of identifying behaviors and design decisions with security implications.
CryDRs are intended to be ERC20-compliant representations of off-chain assets. JNTs are the tokens created in order for Jibrel to levy fees on transactions of CryDRs.
Jibrel uses the Model- View-Controller (MVC) pattern to allow the business logic to be updated without requiring that users interact with a new contract address or needing to copy the state of the previous contract. Due to the Know Your Customer (KYC) and Anti-Money Laundering (AML) regulations involved in trading real-world assets, there also exists an investor registry that only allows for certain CryDRs to be traded amongst addresses that have been verified by Jibrel.
At the time of the audit, the Board of Directors Smart Contract and DAO contract appear to not be completed as the code does not implement functionality to manage members or proposals.
Files Audited
Jibrel provided New Alchemy with access to the following GitHub repository, which was listed as private during the time of the audit: https://github.com/jibrelnetwork/jibrel-contracts
The auditors reviewed the commit of the development branch with a sha1 hash of 25cfb34da829b4b02c3f3eaf5c069a276d113dd5.
The code imports zeppelin-solidity version 1.2.0.
General Discussion
The code specifies Solidity version 0.4.15, which is older than the the recommended latest version at the time of the audit, which is 0.4.17. Jibrel should use the latest version of Solidity in order to take advantage of the latest features and improvements that Solidity has to offer.
The code uses the Truffle framework for migrations and testing. While some unit tests exist for the contracts, the unit tests are not sufficiently thorough in testing the full functionality of the various contracts. Generally the unit tests cover the functionality directly exposed to customers and the standalone behavior of the supporting contracts. For example, the Manageable.sol contract is tested to ensure that an address’s permissions can be managed and validated but there is no testing to ensure that the functions with dangerous functionality that should require a permission actually requires a permission. While New Alchemy did not identify any issues that could have been prevented by this more extensive testing, this will prevent access control issues in the future that could be the result of mistakenly deleting a modifier. Additionally, the functionality that requires permissions to execute is not tested, which means that these functions may not behave as expected when they are deployed.
Most of the contracts that will be deployed inherit from the Pausable contract, which allows manager accounts with permissions to pause and unpause contracts to stop users from making certain actions while the contract is being managed or under attack. Exceptions to this rule are contracts such as the investor registry in which users without manager permissions cannot change the state in any way.
It should be noted that the code is very modular in design and makes frequent use of inheritance. This has the advantages of making large codebases easier to manage and reduces the amount of repeated code. The major drawback to a heavily inherited design such as this is that it opens up the possibility of introducing confusing issues related to multiple inheritance in addition to being more difficult to read and identify all of the possible entry points for a contract. With that stated, the auditors did not identify any issues as the result of this design choice.
Jibrel establishes an account as the owner of each contract upon deployment which then delegates another Jibrel-owned account as manager with various permissions over the contracts. Some of the permissions defined by the contracts allow for Jibrel to replace the controller for a CryDR to allow Jibrel to update the business logic of their contracts after deployment. While this means that Jibrel is able to respond to bugs, it also means that Jibrel can change the behavior of the contracts in any way they would like after investors have purchased JNTs and CryDRs. Similarly, Jibrel has the power to mint arbitrary tokens of any type but, due to the limitations of smart contracts, there is no way to validate that there are real off-chain assets corresponding to the minted tokens.
Jibrel implemented an investor registry contract which limits the trade of some CryDRs based on whether the various parties have been approved by Jibrel. New Alchemy did not evaluate the off-chain verification process and cannot attest to Jibrel’s adherence to financial regulations.
Major & Critical Issues
No major or critical issues were found.
Moderate and Minor Issues
Moderate / Minor issues flagged during the Security Audit
Insufficient mitigations against double-spend attacks in approve and transferFrom functions: The JNTController contract is susceptible to the attack described in https://github.com/ethereum/ EIPs/issues/20#issuecomment-263524729. In this attack, an attacker attempts to spend their allowance between the point where the victim user checks the attacker’s allowance and the point where the victim’s transaction adjusting the attacker’s allowance is mined. This means that the potential exists for attackers to spend the sum of the existing allowance and the desired allowance every time a user approves an allowance for them.
Apparently, Jibrel realizes this issue and successfully mitigated it in the CrydrControllerSingleLicense and CrydrControllerNoLicense contracts by ensuring that the allowance is either zero or being set to zero before going through with the call. In the JNTController, however, they only check that the attacker’s allowance is greater than or equal to zero prior to setting their allowance. Unfortunately, this attempted mitigation does not actually provide any protections as any unsigned integer will meet these requirements.
This inconsistent mitigation implementation could be avoided by moving the common logic for the transfer, approve, and transferFrom functions to a CrydrControllerERC20 contract from which the various controllers will be inherited.
The mitigation in which investors must set an allowance to zero before changing it to another value does not break the ERC20 standard but helps to prevent users from being affected by a double-spend attack. Unfortunately, this mitigation does not protect users who send transactions setting the allowance to zero and then immediately setting the allowance to the intended value without checking if the attacker spent their allowance before the allowance was reset to zero. Jibrel should consider providing functions that are not included in the ERC20 standard that are entirely protected against these race condition issues for users to use. Examples of this could include increaseAllowance and decreaseAllowance functions or an approve function which takes a third parameter representing the original allowance.
Lack of short address attack protections: Users who wish to transfer CryDRs or JNTs may be vulnerable to a short address attack if they do not verify the length of the address that they send to the transfer, approve, or transferFrom functions. Due to the way that the EVM works, if a user is coerced into sending an address that is not long enough, the remaining bytes will be filled with the beginning bytes of the next argument and the next argument will be shifted over by the number of missing bytes. This means that an attacker can generate a wallet address ending with “00” and submit their address without the “00” at the end to another user in the hopes that they will fill in the short address and shift the value of the transfer over by the number of bytes missing from the address. As a result users, particularly automated ones like exchanges, may be tricked into transferring a much larger number of tokens than they intend. The length of the input data should be checked on all function calls where this attack vector may be relevant.
Two-step owner transfers: Ownership transfers are currently a one stop process in which the owner simply provides the address of the new owner. This sort of behavior can be dangerous because a mistake when entering the address could result in a contract without an owner. A preferred set of behaviors would be to require the new owner to approve the transfer of ownership before the owner is actually changed.
JSON injection: The CryDRRegistry and InvestorRegistry contracts define functions which return data in storage formatted as JSON for front ends to parse and use. The strings in the data returned are not escaped before being placed in the returned JSON object, which allows for JSON injection. This is currently an unlikely attack vector as the only strings that can be used for this injection include license names and token names and symbols, which are both provided by Jibrel. If, by some means a malicious party is able to convince Jibrel to input a specially crafted value for one of those strings, the JSON could be unreliable to clients. An example of this could be an attacker who is in some way able to influence Jibrel to register a CryDR with a custom symbol that would add a second registered CryDR to the JSON results without adding the other CryDR to the actual registry. If a CryDR is registered with the following token symbol, then a client reading the JSON would see an extra CryDR named “fakecoin”:
FAKE", "name": "fakecoin", "views": [{"apistandard": "erc20", "address":
"0x0123456789abcdef0123456789abcdef"}]},{"symbol": "realSymbolThis represents a privilege escalation from controlling the symbol of a CryDR to adding an extra CryDR to the JSON results of the getCryDRData function.
Line by line comments
Line by line comments from the Security Audit are omitted for security purposes. These will be released once the Jibrel Network team are confident that all security issues have been addressed, and we have undertaken our remaining two Security cross-Audits (undertaken by Hacken and Zeppelin Solutions).
Conclusion
While the audit itself makes no warranties or guarantees on the safety of the code base, the Jibrel Network has engaged sufficient vendors for audits and cross-audits to ensure the contracts deployed to power the network contain new critical security flaws.
Jibrel provides traditional financial assets such as currencies, commodities, debt instruments and securities, as standard ERC-20 tokens, on the Ethereum blockchain. Jibrel is a Jibrel AG initiative. Jibrel AG is registered in Zug, Switzerland, Qubist Labs Inc is a software development company based out of New York, US.
