Ocean Protocol v3

CertiK8 Oct 2020Files named in the report

CertiK reviewed Ocean Protocol (Ocean Protocol v3) on 8 Oct 2020: 60 findings, 0 critical; 30 marked fixed, 30 acknowledged.

No open critical findings

Open the report Open Ocean Protocol’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings60

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

Severity	Acknowledged	Fixed
Critical	0	0
High	1	0
Medium	0	0
Low	29	30

HighInvalid AssumptionAcknowledged

The _viewlock_ modifier and the way it is utilized infer that the functions it guards will be uninvokable when the reentrancy mutex has been placed on the contract. While this is true, they will not be "unviewable" as it would still be possible for an attacker to gain access to the underlying data the function's protect via low level assembly calls. As such, its usefulness can be disputed as it can ultimately be bypassed.

LowCalculation OptimizationsAcknowledged

The whole contract could be further optimized.

LowCompatibility NoticeAcknowledged

The linked function implementations are correct in evaluating whether the transfers were successful or not, however certain ERC20 tokens incorrectly implement the specification by ultimately not returning any variable.

Lowdelete Instead of Empty AssignmentAcknowledged

The linked statement assigns a zeroed out struct to the mapping location of the unbound token.

LowDocumentation of FunctionalityAcknowledged

The functions bmul and bdiv conducts a ceiling operation on the result which is undocumented.

LowERC-20 Compatibility NoticeAcknowledged

LowExplicit return of Named Return VariableAcknowledged

The spotPrice is a named return variable, meaning that whatever value it holds will be automatically returned at the end of the function's execution. On the last statement of calcSpotPrice , the final value the function is meant to return is assigned to the return variable as well as returned explicitly.

LowFunctionally Equivalent GettersAcknowledged

The two linked getters return the exact same variables albeit the latter of the two imposes a require check prior to fulfilling the getter request.

LowGeneral CommentAcknowledged

No findings were identified at this stage of the audit.

LowInconsistent SpecificationAcknowledged

The specification of the calcSingleInGivenPoolOut function states that the denominator of the division is equal to 1, denoted by the constant BONE , subtracted by the normalized weight (weight in divided by total weight) and finally multiplied by the swap fee. This is not aligned with the actual calculations carried within the function as a final additional subtraction occurs between the final result of the above equation and once again the value of 1 denoted by BONE , the value being subtracted from BONE .

Show 10 more

LowIncorrect CommentAcknowledged

The linked comment contains a discrepancy with the actual implementation that accompanies it. The comment implementation contains a subtraction with 1 whereby the Solidity implementation contains an addition.

LowIncorrect Variable LabelsAcknowledged

The function setController is meant to replace the existing controller address with a new one, however the new one is labelled as manager and its accompanying error message also states ERR_INVALID_MANAGER_ADDRESS which is not the case as we are handling a controller at this point.

LowInvalid DocumentationAcknowledged

The getCurrentTokenIndex function, as its comments dictate, should return the index of the current token. However, the variable returned is actually initialized at 1 and is named currentTokenCount .

LowMisleading CommentAcknowledged

The comment denotes that the index member of the struct is private , a concept that does not exist within Solidity in the same terms it exists in other languages. Any contract can arbitrarily read the storage of another contract regardless of what access control restrictions are imposed, meaning that this comment can be misleading to an unaware reader

LowNecessity of Function VisibilityAcknowledged

The `createToken` function is declared as `public` yet remains unused by any of the contracts within the project.

LowPull-over-Push PatternAcknowledged

Directly replacing the managerial address of a contract is dangerous as a single misinputed character can freeze the administrative functions of the contract indefinitely.

LowRedundant require ChecksAcknowledged

The linked require checks are redundant as they are checked by the underlying implementation of bsub .

LowRedundant Type CastingAcknowledged

The variables utilized are already of type address and are redundantly casted to that data type.

LowRedundant Value AssignmentAcknowledged

The linked initialized variable is a bool data type. In Solidity, all data types are automatically assigned their default value, in the case of a bool being false , meaning that the explicit assignment of false to the variable is redundant.

Lowrequire to modifierAcknowledged

The linked require statements are replicated multiple times across the contact and can instead be set as re-usable modifiers .

Show 10 more

LowStruct OptimizationAcknowledged

The index variable is declared as a uint which is a shorthand of uint256 whilst it is meant to represent values between 0 and 8 , according to the imposed limit of BConst.sol ( MAX_BOUND_TOKENS being equal to 8 ). Since this number will always realistically fit even within a uint8 representation never exceeding the value of 255 it is possible to adjust its data type to tightly pack it with the preceding bool variable bound reducing the total storage cost of the struct from 4 slots to 3, significantly optimizing gas cost.

LowUnusual Naming ConventionAcknowledged

The naming convention utilized for the linked modifiers does not conform to the Solidity style guide as it includes the underscore ( _ ) character on both ends of the name declaration.

LowUser Defined GetterAcknowledged

In general, it is a good practise to avoid user-defined getters and instead rely on compilergenerated ones via the `public` keyword as they are far more maintainable and aid in the legibility of the codebase.

LowUser Defined GettersAcknowledged

In general, it is a good practise to avoid user-defined getters and instead rely on compilergenerated ones via the public keyword as they are far more maintainable and aid in the legibility of the codebase.

LowUser Defined GettersAcknowledged

LowValue Based RefundAcknowledged

The current implementation utilizes a value-based refund instead of a proportional refund.

LowVariable Mutability SpecifiersAcknowledged

The linked variables are only assigned to once in their actual declaration.

LowVariable Naming ConventionAcknowledged

While the documentation of the functions was extremely helpful in properly evaluating the intended purpose of the functions, the variable names utilized within (foo, bar, zaz etc.) are generally utilized as placeholders and are ill-advised to exist in production-ready code.

LowVariable OrderingAcknowledged

Within Solidity, the order of variable declaration is important within a contract as it dictates how the compiler will tightly-pack the variables it is provided with. While the layout of the contract does not appear to follow a particular convention, it has tightly packed most slots possible except one.

LowVariable UsefulnessAcknowledged

Throughout the codebase, these two input variables appear to be hard-coded as false when passed to the function call.