Bancor Liquidity

CertiK9 Oct 2020Files named in the report

Audit

CertiK reviewed Bancor (Bancor Liquidity) on 9 Oct 2020: 17 findings, 0 critical; 6 marked fixed, 11 open.

No open critical findings

Open the report Open Bancor’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings17

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	0	0
Medium	0	0
Low	11	6

Lowcheck-effect-patterns not followed when sending eOpen

`transfer` or `safeTransferFrom` is done before storage variables are updated.

LowExternal calls inside a loopOpen

External calls in the loop may lead to issues with GAS costs of a transaction.

LowInefficient greater-than comparison w/ zeroOpen

Within Solidity, unsigned integers are restricted to the non-negative range. As such, greater-than comparisons with the literal 0 are inefficient gas-wise.

LowmaxSystemNetworkTokenAmount limit is mutable.Open

According to the documentation, `The system holdings cannot exceed 500K BNT worth of pool tokens in a pool`. In contrast, `maxSystemNetworkTokenAmount` is not a constant value and can be changed using `setSystemNetworkTokenLimits()` function.

LowOld implementation of Ownable functionalityOpen

Implementation of Owned.sol comes from OpenZeppelin version 1.12.0, which is from 2018 and is no longer valid.

LowUnneccessary if statementOpen

If statements can be simplified as `_newConverter.addReserve(ETH_RESERVE_ADDRESS, weight);` is repeated in both cases.

LowUsage of `syncReserveBalances` in the beginning oOpen

`syncReserveBalances` is called at the start of many functions.

LowUsage of uint16 in for loopsOpen

`uint16` is not optimal use for `for loop`.

LowUse of an assertOpen

assert() uses all gas available in the transaction. The assert() function should only be used to test for internal errors and check invariants. Within Solidity, unsigned integers are restricted to the non-negative range. As such, greater-than comparisons with the literal 0 are inefficient gas-wise.

LowUse of transfer() for sending ETHOpen

After EIP 1884 was included in the Istanbul hard fork, it's not recommended to use .transfer() and .send() as this function's a hard dependency on gas costs make them obsolete as they are forwarding a fixed amount of gas 2300 . This can cause some issues when sending ether to a contract. send and transfer do not longer protect from reentrancies in case of gas price changes.

Show 1 more

LowUse of two arrays instead of a mappingOpen

As the `addLiquidity` , `removeLiquidity` and `verifyLiquidityInput` functions all accept an array of token addresses and an array of amounts that are associated with each other, it makes code harder to review and read.