Kyber Network Smart Contracts Security Analysis

In this report, we consider the security of the Kyber Network project. Our task is to find and describe security issues in the smart contracts of the platform.

The audit does not give any warranties on the security of the code. One audit cannot be considered enough. We always recommend proceeding with several independent audits and a public bug bounty program to ensure the security of smart contracts. Besides, a security audit is not an investment advice.

In this report, we considered the security of Kyber Network smart contracts. We performed our audit according to the procedure described below.

The initial audit showed neither critical nor medium severity issues. However, a number of low severity issues were found. They do not endanger project security. Nevertheless, we recommend addressing them.

Some of the issues were fixed in the latest version of the code.

The contracts code is of high code quality. The audit did not reveal any issues that endanger project security.

If the developers decide to improve the code, we recommend fixing the wrong import issue. However, mentioned above is a minor issue. It does not affect code operation.

In our audit, we consider the following crucial features of the smart contract code:

Whether the code is secure;
Whether the code corresponds to the documentation (including whitepaper);
Whether the code meets best practices in efficient use of gas, code readability, etc.

We perform our audit according to the following procedure:

Automated analysis

we scan the smart contracts with our own Solidity static code analyzer SmartCheck;
we scan the smart contracts with several publicly available automated Solidity analysis tools such as Remix, and Solhint;
we manually verify (reject or confirm) all the issues found by tools.

Manual audit

we manually analyze smart contracts for security vulnerabilities.

Report

we reflect all the gathered information in the report.

We have scanned BlockState smart contracts for commonly known and more specific vulnerabilities. Here are some of the commonly known vulnerabilities that we considered (the full list includes them but is not limited to them):

Project description

In our analysis we consider Kyber Network specification (”README.md” in the repo) and smart contracts’ code (private repository, version on commit 4cc9ca425f31323ff313b9ea99429c5b6104d612).

The latest version of the code

After the initial audit, some fixes were applied and the code was updated to the latest version (private repository, version on commit 80f7457c93aa75cbf92213718d257617678426e9).

Project architecture

For the audit, we were provided with the truffle project. The project is an npm package and includes tests.

The project successfully compiles with truffle compile command (see Compilation output in Appendix)
The project successfully passes all the tests with 100% coverage

The total LOC of audited Solidity sources is 281.

We used several publicly available automated Solidity analysis tools. All the issues found by tools were manually checked (rejected or confirmed). Cases, when these issues lead to actual bugs or vulnerabilities, are described in the next section.

The contracts were completely manually analyzed, their logic was checked and compared with the one described in the documentation. Besides, the results of the automated analysis were manually verified. All confirmed issues are described below.

Critical issues seriously endanger smart contracts security. We highly recommend fixing them.

The audit showed no critical issues.

Medium issues can influence smart contracts operation in the current implementation. We highly recommend addressing them.

The audit showed no medium severity issues.

Low severity issues can influence smart contracts operation in future versions of code. We recommend taking them into account.

Missing input validation (fixed)

There is a missing check at KyberSwapLimitOrder.sol, lines 147–149:

function invalidateOldOrders(…) public {
require(isValidNonce(…));
updateNonce(…);

User can insert an incorrect nonce with a different contract address prefix that satisfies isValidNonce check. After that, executeLimitOrder function will always revert due to check at KyberSwapLimitOrder.sol, line 161:

require(validAddressInNonce(verifyParams.nonce));

Thus, we recommend adding this check into invalidateOldOrders function.

Unsafe array’s length manipulation

There are several unsafe array’s length manipulations.

PermissionGroups.sol, line 94:

alertersGroup.length--;

PermissionGroups.sol, line 119:

operatorsGroup.length -= 1;

In these cases, there are corresponding checks to prevent errors. However, we recommend using safe pop() method to improve code readability.

Safe ERC20 (fixed)

There are ERC20 token function calls inside require().

KyberSwapLimitOrder.sol, line 50:

require(token.approve(address(kyberNetworkProxy), 2 ** 255));

KyberSwapLimitOrder.sol, line 174:

require(tradeInput.srcToken.transferFrom(verifyParams.user, address(this), tradeInput.srcQty));

Withdrawable.sol, line 24:

require(token.transfer(sendTo, amount));

Some older ERC20 tokens do not provide any return value when functions are called. Thus, even a correct call to such a contract will fail. We recommend using SafeERC20 library functionality from OpenZeppelin.

Redundant code

The following lines are redundant:

KyberSwapLimitOrder.sol, line 13

bool public tradeEnabled = false;

Default value of boolean type is false. Therefore, this line is redundant.

KyberSwapLimitOrder.sol, line 29

admin = _admin;

This contract inherits PermissionGroups contract with constructor containing the same instruction.

KyberSwapLimitOrder.sol, line 155

function updateNonce(…) internal returns (bool)

Return value of this function is not initialized. Moreover, it is not used anywhere.

operators and alerters groups are unused in KyberSwapLimitOrder contract. Therefore, this functionality can be omitted.

We highly recommend removing redundant code in order to improve code readability and transparency and decrease the cost of deployment and execution.

Unused return (fixed)

There is an unused return variable at KyberSwapLimitOrder.sol, line 190:

kyberNetworkProxy.tradeWithHint(…);

This function returns uint variable that is not used. We recommend processing this variable and passing its value to LimitOrderExecute event.

Wrong import of OpenZeppelin library

In the current implementation, OpenZeppelin files are added to the repo. This violates OpenZeppelin’s MIT license, which requires the license and copyright notice to be included if its code is used. Moreover, updating code manually is error-prone.

We highly recommend using npm in order to guarantee that original OpenZeppelin contracts are used with no modifications. This also allows for any bug-fixes to be easily integrated into the codebase.

Constant states (fixed)

There are a few variables with unchanging values.

KyberSwapLimitOrder.sol, line 15:

uint256 public maxDestAmount = 2 ** 255;

KyberSwapLimitOrder.sol, line 16:

uint256 public precision = 4;

We recommend using constant keyword in order to increase code readability and reduce gas costs.

External function

There are functions that should be declared external.

KyberSwapLimitOrder.sol, line 33:

function enableTrade() public onlyAdmin

KyberSwapLimitOrder.sol, line 37:

function disableTrade() public onlyAdmin

KyberSwapLimitOrder.sol, line 41:

function listToken(ERC20 token) public onlyAdmin

KyberSwapLimitOrder.sol, line 75:

function executeLimitOrder(…) public nonReentrant

KyberSwapLimitOrder.sol, line 147:

function invalidateOldOrders(…) public

PermissionGroups.sol, line 47:

function transferAdmin(address newAdmin) public onlyAdmin

PermissionGroups.sol, line 57:

function transferAdminQuickly(address newAdmin) public onlyAdmin

PermissionGroups.sol, line 69:

function claimAdmin() public

PermissionGroups.sol, line 78:

function addAlerter(address newAlerter) public onlyAdmin

PermissionGroups.sol, line 87:

function removeAlerter (address alerter) public onlyAdmin

PermissionGroups.sol, line 103:

function addOperator(address newOperator) public onlyAdmin

PermissionGroups.sol, line 112:

function removeOperator (address operator) public onlyAdmin

If a function is not supposed to be called by the contract itself, consider declaring it as external.

Violation of Checks-Effects-Interaction pattern (fixed)

The Checks-Effects-Interaction pattern is violated at KyberSwapLimitOrder.sol, lines 179–190:

kyberNetworkProxy.tradeWithHint(…); updateNonce(…);

In this case, the CEI violation does not lead to an actual vulnerability. However, we highly recommend following best practices since it helps to avoid many serious vulnerabilities.

Code logic (fixed)

There is a hardcoded value at KyberSwapLimitOrder.sol, line 50:

require(token.approve(address(kyberNetworkProxy), 2 ** 255));

Using 2 ** 256 - 1 is preferable because some token contracts optimize transferFrom() function calls in this case. Moreover, we recommend using maxDestAmount constant instead of hardcoded value.

Published at Thu, 18 Jul 2019 14:12:12 +0000

Bitcoin Pic Of The Moment
If you enjoy my photos, you are welcome to #‎donate #‎bitcoin to me at: 1Q2LV3bsxZjRBQoRXAXikpUGPCrNeGSUWc
By antwerpenR on 2013-08-25 13:19:13