Celer Network

CertiK23 Oct 2020Files named in the report

Audit

CertiK reviewed Celer Network on 23 Oct 2020: 49 findings, 0 critical; 33 marked fixed, 16 open.

No open critical findings

Open the report Open Celer Network’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings49

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

Severity	Open	Fixed
Critical	0	0
High	0	0
Medium	0	0
Low	16	33

Low`require` statement can be substituted with a funFixed

The require statement on the aforementioned line asserts that the new commission rate should be less-than-orequal to the already set commission rate. However, the function name nonIncreaseCommissionRate implies that the new rate should be less-than the already set rate. Also, the execution of the function will be ineffectual and nonstate changing if the provided rate and time in the parameters is same as the already set values

Low`require` statement can be substituted with a modFixed

The require statements on the aforementioned lines have the same predicates and error message. The require statement can be converted into a modifier to avoid code duplication and increase legibility of the code

Low`TODO` commentFixed

The aforementioned line has a TODO which we advise that be removed from code.

LowCan be declared constant and use uint256 insteadFixed

The variable declaration on the aforementioned line be changed to a constant to save gas cost associate with storage slot. Additionaly, the type of declaration is uint which is an alias uint256 . A clean coding practice is to use complete type name of uint256 instead of alias uint .

LowConfusing Variable NameFixed

The variable name yesVotes on the aforementioned line implies that it represents the count of yes votes yet it specifies the amount of total stakes represented by validators with yes votes. The variable name should reflect the staking amount it represents.

LowFunction vibility can be changed to externalFixed

The functions on the aforementioned lines are never called from within the contract and hence their visibilities can be changed to external .

LowFunction vibility can be changed to externalFixed

The functions on the aforementioned lines are never called from within the contract and hence their visibilities can be changed to external .

LowGrammar ErrorFixed

The aforementioned line has a grammar error where it says Delegators has to wait .

LowIncorrect ComparisonFixed

The require statement on the aforementioned line checks against that the servicePool should always be greater than newServiceReward which will result in not being able to claim all of funds specified by servicePool .

LowIneffectual Library import and declarationFixed

The library ECDSA is imported and declared but is never used in the contract. The import and declaration on the aforementioned lines can be safely removed.

Show 10 more

LowInefficient CodeFixed

The code on the aforementioned lines can be optimized by the use of a local variable and a single _updateDelegatedStake call to reduce the bytecode footprint of the contract.

LowInefficient CodeFixed

The code on the aforementioned lines only effectually executes when the function _updateCommissionRate is called from confirmIncreaseCommissionRate as part of announced rate update. When the function _updateCommissionRate is called from nonIncreaseCommissionRate , the execution of aforementioned lines is ineffectual and hence costs unwanted gas. The aforementioned lines can be moved to confirmIncreaseCommissionRate after the call to _updateCommissionRate so that the execution of nonIncreaseCommissionRate does not execute the code on aforementioned lines.

LowInefficient usage of SafeMathFixed

The Addition and Subtraction operations on the aforementioned lines do not depend upon the values received outside of the contract and hence their chances of oveflowing or underflowing are neglibible considering a uint256 can store a large enough value for their use cases. These operations can be safely performed without SafeMath which will save the additional gas cost associated with Safe Addition and Safe Subtraction without comprising the security of the contract.

LowInefficient use of local variableFixed

The declaration of local variable on the aforementioned line is inefficient as the variable is never used at more than one places. The gas cost associated with local variable declaration can be saved by using the initialization part directly in place of the declared local variable.

LowInefficient use of local variableFixed

The declaration of local variables on the aforementioned line is inefficient as any variable is never used at more than one place. The gas cost associated with local variable declaration can be saved by using the initialization part directly in place of the declared local variable

LowInexistence of reason in Require StatementsFixed

The require statements on the aforementioned lines do not specify reason string . The reason string s should be provided to require statements to aid debugging and readability of the code.

LowMissing check for the valid inputFixed

The functions on the aforementioned lines has parameter _vote of type VoteType . The code does not have a check against a VoteType value of VoteType.UnVoted , which if passed would successfully execute the transaction without changing the state of the vote

LowPotentially Incorrect ComparisonFixed

The require statement on the aforementioned line asserts that block.number should be greater than the earliestBondTime for the earliest bond time to arrive. However, intiuitively when the block.number is equal to earliestBondTime then it should be considered that the bond time has arrived.

LowPotentially Incorrect Error MessageFixed

The error message on the aforementioned line has a potentially incorrect error message Fail to check validator sigs which implies that the operation of validating signatures itself failed to initiate. The require statement throws when the signatures are invalid and as such the error message should highlight it.

Show 10 more

LowPotentially Incorrect ImplementationFixed

The function confirmSidechainProposal does not handle the assignment of deposited funds when a sidechain proposal does not pass. In the function confirmParamProposal , when a Param proposal does not pass, the deposited funds are assigned to miningPool but no such assignment of funds is implemented in confirmSidechainProposal .

LowRedundant initialization with default valueFixed

The aforementioned line declares variablesof type bool and initialize it with false . In Solidity, all un-initialized variables have a default value which for the bool variable is false , hence the initialization part is redundant and can be removed.

LowRedundant initialization with default valueFixed

The aforementioned lines declare variables of type uint256 and initialize it with 0 . In Solidity, all un-initialized variables have a default value which for the uint256 variable is 0 , hence the initialization part is redundant and can be removed.

LowRedundant State VariableFixed

The variable celerToken on the aforementioned line is initialized with the same address as governToken variable in the parent Govern contract on L71 . As both the variables are initialized with the same ERC-20 token address, one of them can removed to save gas costs associated with additional storage slot and to avoid the duplicate code to achieve code legibility.

LowSpelling ErrorFixed

The aforementioned line has a spelling error for misbehaviour

LowSpelling ErrorFixed

The aforementioned line has a spelling error for the word creation .

LowSpelling ErrorFixed

The comment on the aforementioned line has a spelling error in the word Owner .

LowSubstitution of require calls with ModifierFixed

The require statements on the aforementioned lines can be converted to a modifier to avoid code duplication and increase legibility of the code.

LowThe comment on the aforementioned line has spelliFixed

The comment on the aforementioned line has spelling error for the word Owner .

LowUnlocked Compiler VersionFixed

The contracts have unlocked compiler versions. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one

Show 3 more

LowUsage of ether instead of Decimals MultiplierFixed

The modifier usage on the aforementioned passes minimum of token as 1 CELR . The decimals multiplier used is ether global variable which represents 18 decimals. Although, it is functionally correct but we advise that actual decimals multiplier of the token be used instead of ether global variable for better readability of the code.

LowUsage of storage Variable Instead of memoryFixed

The local variables on the aforementioned lines are declared as storage yet no write operations are performed. The usage of local storage variables is optimal in read operations only when the count of read operations is less than 4. As both of the aforementioned local storage variables perform more than 3 read operations, their data location can be changed to memory as it costs significantly less gas to read from memory . Although, the usage of memory data location will copy the data to memory but it is still cheaper than to make several read operations on storage .

LowVariable not named in camelCaseFixed

The state variable DPoSContract of type IDPoS is not named in camelCase which violates the best coding practices of Solidity.