CoinFabrik was asked to audit the contracts for the EMDX project. First, we will provide a summary of our discoveries and then we will show the details of our findings.
The contracts audited are from the EMDX repository at https://github.com/emdx-dex/perpetual-protocol/. The audit is based on the commit ca34a5e9e254caca5056ab5002a3fdb0b4f3b004.
Fixes were made and rechecked based on the commits:
- ee9bb77d8a96ec409970d795d6eb8b40acba5446 (typos)
The audited contracts are:
- src/ChainlinkL1.sol: Chainlink Oracle
- src/L2PriceFeed.sol: Asset price feed
- src/ClearingHouse.sol: Core perpetual
- src/InsuranceFund.sol: Insurance fund
- src/interface/Iark.sol: Ark interface
- src/Ark.sol: Minter replacement to cover Insurance fund collateral depletion.
Contracts are a fork of the Perpetual protocol (https://github.com/perpetual-protocol/perpetual-protocol), except for IArk.sol and Ark.sol.
The following analyses were performed:
- Misuse of the different call methods
- Integer overflow errors
- Division by zero errors
- Outdated version of Solidity compiler
- Front running attacks
- Reentrancy attacks
- Misuse of block timestamps
- Softlock denial of service attacks
- Functions with excessive gas cost
- Missing or misused function qualifiers
- Needlessly complex code and contract interactions
- Poor or nonexistent error handling
- Failure to use a withdrawal pattern
- Insufficient validation of the input parameters
- Incorrect handling of cryptographic signatures
Findings and Fixes
Security risks are classified as follows:
- Critical: These are issues that we manage to exploit. They compromise the system seriously. They must be fixed immediately.
- Medium: These are potentially exploitable issues. Even though we did not manage to exploit them or their impact is not clear, they might represent a security risk in the near future. We suggest fixing them as soon as possible.
- Minor: These issues represent problems that are relatively small or difficult to take advantage of but can be exploited in combination with other issues. These kinds of issues do not block deployments in production environments. They should be taken into account and be fixed when possible.
- Enhancement: These kinds of findings do not represent a security risk. They are best practices that we suggest implementing. This classification is summarized in the following table:
Issues Found by Severity
No issues found
ME-01 Denial of Service in
In the function
Ark.withdrawForLoss() it is possible to withdraw more than the total balance of the token. This should be prevented by the required statement at line 46 of Ark.sol:
require(_balanceOf(_quoteToken, address(this)).toUint() >= _amount.toUint(), "insufficient funds");
But in the next step in Ark.sol, line 50, the amount to withdraw is incremented by one to compensate for the method
DecimalERC20 that rounds down in the conversion from decimal to uint256. But if the token has 18 or more decimals, the conversion to uint256 is done at line 103 in DecimalERC20.sol:
We can see that in this case, the conversion is simply a multiplication, so no rounding down is done. Subsequently, by increasing the amount by one at line 50 in Ark.sol, the amount to withdraw is now bigger than the balance, and
the _transfer() will fail.
The assumption that Decimal to uint256 conversion always rounds down the value is incorrect, as this only happens with tokens with less than 18 decimals. Adjust the code to reflect this condition or require only tokens with less than 18 decimals.
The issue was fixed by the development team by compensating the amount only when necessary.
MI-01 Invalid calculation in
Ark.withdrawForLoss() it is possible to operate with different ERC20 tokens that are passed as parameter
There are two issues when operating with different tokens:
1) At line 57 in Ark.sol a log of the transfers is calculated in this way:
cumulativeAmount = withdrawnTokenHistory[len - 1].cumulativeAmount.addD(_amount);
This calculation doesn’t consider that
_amount might represent amounts of different tokens. This issue is not serious because the variable
withdrawnTokenHistory is currently unused.
2) At line 64 in Ark.sol we can see the transfer and associated event being generated:
_transfer(_quoteToken, _msgSender(), _amount); emit WithdrawnForLoss(_msgSender(), _amount.toUint());
We can see that the transfer specifies the token type, but the event does not, so any listener of this event has no way to know which token was withdrawn.
Refactor the code to support operation with different tokens, or limit the contract to operate on a single token type.
The issue was fixed by the development team by supporting different tokens in the event and in the token history.
MI-02 Oracle manipulation on
In ChainlinkL1.sol line 118 we can see a decimal conversion calculation:
This exponentiation operation will overflow in any token with more than decimals, causing an exception in the
The same overflow was detected in the following contracts but are out of the scope of this audit, and the impact was not analyzed:
Require a limit or a range for token decimals accepted by this contract.
The issue was mitigated by the fact that the dev team won’t use tokens with more than 18 decimal places.
MI-03 Solidity Compiler Version
All audited files use the pragma solidity 0.6.9; statement. This solidity version is outdated and may introduce bugs. See https://swcregistry.io/docs/SWC-103.
Update the solidity compiler to the latest revision (0.8.10 at the current time). Also, when updating to 0.8 take into account the new semantics for safe math operations.
The issue was acknowledged by the dev team, but it was decided to keep using the compiler because refactoring it is too costly at this time.
EN-01 Uninitialized Contract address
At line 86 in L2PriceFeed.sol, the modifier
onlyChainlink() restricts the call to
chainlinkContract, previously set calling
ChainlinkL1.setChainlink(). However, it is not verified or assured in any way that this function was called, as a result the
chainlinkContract can be uninitialized, preventing this function from updating the prices until the
setChainlink() function is called.
chainlinkContract address in the initializer of the contract, so it never can be used uninitialized.
The issue was contemplated in the deployment script and thus mitigated.
Documentation is inconsistent throughout the source code. The core contract ClearingHouse.sol has good documentation of functions and arguments, but it’s not as detailed in Ark.sol, ChainlinkL1.sol, InsuranceFund.sol and L2PriceFeed.sol.
The contract ChainlinkL1.sol allows the owner to change the price oracle, and in the case of Ark.sol it allows the owner of the contract to withdraw all funds using the
claimTokens() function. Also, all contracts are upgradeable by the owner.
Users of the EMDX protocol need to trust the owner of the contract because of this fact.
We found 1 medium-severity issue and 3 minimal-severity issues. Also, we suggested two enhancements to the code. A medium-severity issue and a minor-severity issue were fixed. A minor-severity issue and an enhancement were mitigated. Finally, a minor-severity issue was acknowledged but a fix was not considered necessary by the development team.
Disclaimer: This audit report is not a security warranty, investment advice, or approval of the EMDX project since CoinFabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.