You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is the top-ranked automated findings report, from The_Madaladinator bot. All findings in this report will be considered known issues for the purposes of your C4 audit.
Utilizing an externally owned account (EOA) as the owner of contracts poses significant dangers of centralization and represents a vulnerable single point of failure. A single private key is susceptible to theft during a hacking incident, or the sole possessor of the key may encounter difficulties in retrieving it when required. It is advisable to contemplate transitioning to a multi-signature arrangement or implementing a role-based authorization framework.
Not all IERC20 implementations revert when there's a failure in approve. The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything.
[M-03] Contracts are completely non-functional due to incompatible Solidity version with Arbitrum
The Shell Protocol V2 is currently live on Arbitrum mainnet (see https://app.shellprotocol.io/). This audit is for V3 and uses version 0.8.20 of Solidity whereas V2 used version 0.8.10 (see Shell github). The compiler for Solidity 0.8.20 switches the default target EVM version to Shanghai, which includes the new PUSH0 op code. This op code may not yet be implemented on all L2s, so deployment on these chains will fail. See this relevant issue on the official Solidity github for reference. As the contract(s) are intended to be deployed on Arbitrum, this will cause them to be completely non-functional. To work around this issue, use an earlier EVM version, such as 0.8.19.
[L-01] User facing functions should have address(0) checks
Parameters of type address in your functions should be checked to ensure that they are not assigned the null address (address(0x0)). Failure to validate these parameters can lead to transaction reverts, wasted gas, the need for transaction resubmission, and may even require redeployment of contracts within the protocol in certain situations. Implement checks for address(0x0) to avoid these potential issues.
Prior to Solidity 0.8.0, pressing a confirm consumes the remainder of the process's available gas instead of returning it, as request()/revert() did. assert() and ruqire(); The big difference between the two is that the assert()function when false, uses up all the remaining gas and reverts all the changes made. Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling. Assertion() should be avoided even after solidity version 0.8.0, because its documentation states "The Assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake".
[L-03] Lack of two-step update for critical functions
A copy-paste error or a typo may end up bricking protocol functionality, or sending tokens to an address with no known private key. Consider implementing a two-step procedure for critical functions, where the recipient is set as pending, and must "accept" the assignment by making an affirmative call. A straight forward way of doing this would be to have the target contracts implement EIP-165, and to have the "set" functions ensure that the recipient is of the right interface type.
There are 1 instance(s) of this issue:
File: src/ocean/Ocean.sol
196: function changeUnwrapFee(uint256nextUnwrapFeeDivisor) externaloverride onlyOwner {
[L-04]decimals() is not part of the ERC20 standard
The decimals() function is not a part of the ERC-20 standard, and was addedlater as an optional extension. As such, some valid ERC20 tokens do not support this interface, so it is unsafe to blindly cast all tokens to this interface, and then call this function.
[L-05] Some tokens may revert when zero value transfers are made
In spite of the fact that EIP-20 states that zero-valued transfers must be accepted, some tokens, such as LEND will revert if this is attempted, which may cause transactions that involve other tokens (such as batch operations) to fully revert. Consider skipping the transfer if the amount is zero, which will also save gas.
If the intention is for the ETH to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))).
[L-07] Some tokens do not consider type(uint256).max as an infinite approval
Some tokens such as COMP downcast such approvals to uint96 and use that as a raw value rather than interpreting it as an infinite approval. Eventually these approvals will reach zero, at which point the calling contract will no longer function properly.
[L-08] Contracts use infinite approvals with no means to revoke
Infinite approvals on external contracts can be dangerous if the target becomes compromised. See here for a list of approval exploits. The following contracts are vulnerable to such attacks since they have no functionality to revoke the approval (call approve with amount 0). Consider enabling the contract to revoke approval in emergency situations.
[L-10] Checks-Effects-Interactions pattern not followed
The Checks-Effects-Interactions pattern (CEI) is a best practice that reduces the attack surface for reentrancy attacks. To adhere to this pattern, place state variable updates before external calls within functions.
Tokens such as COMP or UNI will revert on approval if the amount exceeds type(uint96).max. Ensure that the calls below can be broken up into smaller batches if necessary.
Tokens such as COMP or UNI will revert on transfer/transferFrom when an address' balance reaches type(uint96).max. Ensure that the calls below can be broken up into smaller batches if necessary.
This project's used OpenZeppelin version (4.8.1) is vulnerable to one or more of the specific CVEs listed below. Consider switching to a more recent version that doesn't have these vulnerabilities.
VulnerabilitiesCVE-2023-26488: The ERC721Consecutive contract designed for minting NFTs in batches does not update balances when a batch has size 1 and consists of a single token. Subsequent transfers from the receiver of that token may overflow the balance as reported by balanceOf. The issue exclusively presents with batches of size 1. The issue has been patched in 4.8.2 (@openzeppelin/contracts >=4.8.0 <4.8.2). CVE-2023-30541: A function in the implementation contract may be inaccessible if its selector clashes with one of the proxy's own selectors. Specifically, if the clashing function has a different signature with incompatible ABI encoding, the proxy could revert while attempting to decode the arguments from calldata. The probability of an accidental clash is negligible, but one could be caused deliberately and could cause a reduction in availability. The issue has been fixed in version 4.8.3. As a workaround if a function appears to be inaccessible for this reason, it may be possible to craft the calldata such that ABI decoding does not fail at the proxy and the function is properly proxied through (@openzeppelin/contracts >=3.2.0 <4.8.3). CVE-2023-30542: The proposal creation entrypoint (propose) in GovernorCompatibilityBravo allows the creation of proposals with a signatures array shorter than the calldatas array. This causes the additional elements of the latter to be ignored, and if the proposal succeeds the corresponding actions would eventually execute without any calldata. The ProposalCreated event correctly represents what will eventually execute, but the proposal parameters as queried through getActions appear to respect the original intended calldata. This issue has been patched in 4.8.3. As a workaround, ensure that all proposals that pass through governance have equal length signatures and calldatas parameters (@openzeppelin/contracts >=4.3.0 <4.8.3). CVE-2023-34234: By frontrunning the creation of a proposal, an attacker can become the proposer and gain the ability to cancel it. The attacker can do this repeatedly to try to prevent a proposal from being proposed at all. This impacts the Governor contract in v4.9.0 only, and the GovernorCompatibilityBravo contract since v4.3.0. CVE-2023-34459 When the verifyMultiProof, verifyMultiProofCalldata, processMultiProof, or processMultiProofCalldata functions are in use, it is possible to construct merkle trees that allow forging a valid multiproof for an arbitrary set of leaves. A contract may be vulnerable if it uses multiproofs for verification and the merkle tree that is processed includes a node with value 0 at depth 1(just under the root). This could happen inadvertently for balanced trees with 3 leaves or less, if the leaves are not hashed.This could happen deliberately if a malicious tree builder includes such a node in the tree. A contract is not vulnerable if it uses single- leaf proving (verify, verifyCalldata, processProof, or processProofCalldata), or if it uses multiproofs with a known tree that has hashed leaves.Standard merkle trees produced or validated with the @openzeppelin/merkle-tree library are safe (@openzeppelin/contracts >=4.7.0 <4.9.2).
Division by large numbers may result in precision loss due to rounding down, or even the result being erroneously equal to zero. Consider adding checks on the numerator to ensure precision loss is handled appropriately.
[L-16] Use a low level call to send ether instead of .send() or .transfer()
The .send() function intends to transfer an ETH amount with a fixed amount of 2300 gas. This function is not equipped to handle changes in the underlying .send() and .transfer() functions which may supply different amounts of gas in the future. Additionally, if the recipient implements a fallback function containing some sort of logic, this may inevitably revert, meaning the vault and owner of the contract will never be able to call certain sensitive functions. Consider using .call() instead with the checks-effects-interactions pattern implemented correctly. Careful consideration needs to be made to prevent reentrancy.
[G-01]abi.encodePacked is more gas efficient than abi.encode
abi.encode pads all elementary types to 32 bytes, whereas abi.encodePacked will only use the minimal required memory to encode the data. See here for more info.
[G-02] Functions guaranteed to revert when called by normal users can be marked payable
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost (2400 per instance).
For example emit ExampleEvent(amount) (amount is uint256) can be re-written as solidity assembly { let memptr := mload(0x40) mstore(0x00, calldataload(0x44)) mstore(0x20, calldataload(0xa4)) mstore(0x40, amount) log1( 0x00, 0x60, // keccak256("ExampleEvent(uint256)") 0x12210f92675543a3eee7d9f6cc64eaca8eb1431502f685da3f48e7593e2b7f1e ) mstore(0x40, memptr) }
Saves 5000 deployment gas per instance and 374 runtime gas per instance. ### Unoptimized solidity function solidityHash(uint256 a, uint256 b) public view { //unoptimized keccak256(abi.encodePacked(a, b)); } ### Optimized solidity function assemblyHash(uint256 a, uint256 b) public view { //optimized assembly { mstore(0x00, a) mstore(0x20, b) let hashedVal := keccak256(0x00, 0x40) } }
Prior to solc 0.8.0, assert used the invalid opcode which used up all the remaining gas while require used the revert opcode which refunded the gas and therefore the importance of using require instead of assert was greater. However, after 0.8.0, assert uses revert opcode just like require but creates a Panic(uint256) error instead of Error(string) created by require. Solidity documentation states: 'The assert function generates an error of type Panic(uint256). Code that works properly should never Panic, even on invalid external input. If this happens, you need to fix it in your contract. there's a mistake'.
By changing the pattern of assigning value to the structure, gas savings of ~130 per instance are achieved. In addition, this use will provide significant savings in distribution costs. Instead of solidity MyStruct memory myStruct = MyStruct(_a, _b, _c); write solidity MyStruct memory myStruct; myStruct.a = _a; myStruct.b = _b; myStruct.c = _c;
In Solidity, unnecessary operations can waste gas. For example, a transfer function without a zero amount check uses gas even if called with a zero amount, since the contract state remains unchanged. Implementing a zero amount check avoids these unnecessary function calls, saving gas and improving efficiency.
[G-11] Cache multiple accesses of mapping/array values
Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata.
[G-13] Use calldata instead of memory for function arguments that are read only
When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gas-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
The expression type(int).min/(-1) is the only case where division causes an overflow. Therefore, uncheck can be used to save gas in scenarios where it is certain that such an overflow will not occur.
[G-16] Stack variable cost less while used in emitting event
Use the function/modifiers local copy of the state variable, rather than incurring an extra Gwarmaccess (100 gas). In the unlikely event that the state variable hasn't already been used by the function/modifier, consider whether it is really necessary to include it in the event, given the fact that it incurs a Gcoldsload (2100 gas), or whether it can be passed in to or back out of the functions that do use it.
Currently, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first).
There are 1 instance(s) of this issue:
File: src/ocean/Ocean.sol
501: for (uint256 i =0; i < interactions.length;) {
The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas. It should be converted to the <=/>= equivalent when comparing against integer literals.
There are 11 instance(s) of this issue:
File: src/ocean/Ocean.sol
419: if (inputAmount >0) {
426: if (outputAmount >0) {
527: if (inputAmount >0) {
533: if (outputAmount >0) {
558: } elseif (mintIds.length>1) {
568: } elseif (burnIds.length>1) {
1009: if (_isNotTokenOfPrimitive(inputToken, primitive) && (inputAmount >0)) {
1043: if (_isNotTokenOfPrimitive(outputToken, primitive) && (outputAmount >0)) {
1084: if (truncated >0) {
1167: if (amount >0) {
[G-22] Expressions for constant values such as a call to keccak256 should use immutable rather than constant
When left as constant, the value is re-calculated each time it is used instead of being converted to a constant at compile time. This costs an extra ~100 gas for each access. Using immutable only incurs the gas costs for the computation at deploy time. See here for a detailed description of the issue.
[G-23] Using storage instead of memory for structs/arrays saves gas
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct.
[G-24] Refactor modifiers to call a local function
Modifiers code is copied in all instances where it's used, increasing bytecode size. By doing a refractor to the internal function, one can reduce bytecode size significantly at the cost of one JUMP.
[G-25] Combine multiple mappings with the same key type where appropriate
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
[G-26] Nesting if-statements is cheaper than using &&
Nesting if-statements avoids the stack operations of setting up and using an extra jumpdest, and saves 6 gas. Note that if an else statement is present, then nesting would use more gas, not less.
There are 12 instance(s) of this issue:
File: src/ocean/Ocean.sol
1009: if (_isNotTokenOfPrimitive(inputToken, primitive) && (inputAmount >0)) {
1010: // Since the primitive consented to receiving this token by not1011: // reverting when it was called, we mint the token without1012: // doing a safe transfer acceptance check. This breaks the1013: // ERC1155 specification but in a way we hope is inconsequential, since1014: // all primitives are developed by people who must be1015: // aware of how the ocean works.1016: _mintWithoutSafeTransferAcceptanceCheck(primitive, inputToken, inputAmount);
1017: }
1043: if (_isNotTokenOfPrimitive(outputToken, primitive) && (outputAmount >0)) {
1044: _burn(primitive, outputToken, outputAmount);
1045: }
[G-27] Function names can be optimized to save gas
public/external function names and public member variable names can be optimized to save gas. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.
Payable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided. A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
[G-30] Not using the named return variable is confusing and can waste gas
Consider changing the variable to be an unnamed one, since the variable is never assigned, nor is it returned by name. If the optimizer is not turned on, leaving the code as it is will also waste gas for the stack variable.
[G-31] Use solady library where possible to save gas
Solady is a Solidity library inspired by Solmate, optimized heavily for gas optimizations and battle tested by hundreds of developers. Consider implementing solady contracts where possible to reduce runtime gas fees.
[G-32] Assigning state variables directly with named struct constructors wastes gas
Using named arguments for struct means that the compiler needs to organize the fields in memory before doing the assignment, which wastes gas. Set each field directly in storage (use dot-notation), or use the unnamed version of the constructor.
[G-34] Usage of uint smaller than 32 bytes (256 bits) incurs overhead
When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. Consider using a larger size then downcasting where needed. https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
[G-36] Avoid updating storage when the value hasn't changed
If the old value is equal to the new value, not re-storing the value will avoid a Gsreset (2900 gas), potentially at the expense of a Gcoldsload (2100 gas) or a Gwarmaccess (100 gas).
There are 6 instance(s) of this issue:
File: src/ocean/Ocean.sol
196: function changeUnwrapFee(uint256nextUnwrapFeeDivisor) externaloverride onlyOwner {
197: /// @notice as the divisor gets smaller, the fee charged gets larger198: if (MIN_UNWRAP_FEE_DIVISOR > nextUnwrapFeeDivisor) revert();
199: emitChangeUnwrapFee(unwrapFeeDivisor, nextUnwrapFeeDivisor, msg.sender);
200: unwrapFeeDivisor = nextUnwrapFeeDivisor;
201: }
Using assembly to check for zero can save gas by allowing more direct access to the evm and reducing some of the overhead associated with high-level operations in solidity.
Enable on the command line using --via-ir or with the option {"viaIR": true} for more powerful optimization passes that span across functions. See here for more info.
Modifiers in Solidity can improve code readability and modularity by encapsulating repetitive checks, such as address validity checks, into a reusable construct. For example, an onlyOwner modifier can be used to replace repetitive require(msg.sender == owner) checks across several functions, reducing code redundancy and enhancing maintainability.
Passing empty bytes to a function can cause unexpected behavior, such as certain operations failing, producing incorrect results, or wasting gas. It is recommended to check that all bytes parameters are not empty.
The contracts should expose an interface so that other projects can more easily integrate with it, without having to develop their own non-standard variants.
[N-10] Contract does not follow suggested layout ordering
Within a contract, the ordering should be: 1. Type declarations 2. State variables 3. Events 4. Modifiers 5. Functions See the Solidity style guide for more info.
[N-11] Control structures do not follow the Solidity style guide
According to the Solidity style guide, braces should open on the same line as the declaration, close on their own line and the opening brace should be preceded by a single space.
[N-12] Take advantage of Custom Error's return value property
An important feature of Custom Error is that values such as address, tokenID, msg.value can be written inside the () sign, this kind of approach provides a serious advantage in debugging and examining the revert details of dapps such as tenderly.
There are 11 instance(s) of this issue:
File: src/ocean/Ocean.sol
186: if (!isApprovedForAll(userAddress, msg.sender)) revertFORWARDER_NOT_APPROVED();
640: if (specifiedAmount !=1) revertINVALID_ERC721_AMOUNT();
648: if (specifiedAmount !=1) revertINVALID_ERC721_AMOUNT();
840: revertNO_DECIMAL_METHOD();
878: revertNO_DECIMAL_METHOD();
929: if (tokenAddress ==address(this)) revertNO_RECURSIVE_WRAPS();
964: if (tokenAddress ==address(this)) revertNO_RECURSIVE_UNWRAPS();
[N-13] Use custom errors rather than require/revert
Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.
There are 3 instance(s) of this issue:
File: src/ocean/Ocean.sol
198: if (MIN_UNWRAP_FEE_DIVISOR > nextUnwrapFeeDivisor) revert();
Complex casting should be avoided in Solidity contracts where possible to prevent unintended consequences and ensure accurate data representation. Performing multiple type casts in succession can lead to unexpected truncation, rounding errors, or loss of precision, potentially compromising the contract's functionality and reliability. Consider adding comments to explain in detail why the casts are necessary, and any implicit reasons why the cast does not introduce an overflow.
Adding a way to quickly halt protocol functionality in an emergency, rather than having to pause individual contracts one-by-one, will make in-progress hack mitigation faster and much less stressful.
When an action is triggered based on a user's action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender the events of these types of action will make events much more useful to end users, especially when msg.sender is not tx.origin.
Index event fields make the field more quickly accessible to off-chain tools that parse events. This is especially useful when it comes to filtering based on an address. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Where applicable, each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three applicable fields, all of the applicable fields should be indexed.
[N-21] Function modifier order does not follow the Solidity Style Guide
The Solidity Style Guide states that the modifier order for a function should be: 1. Visibility 2. Mutability 3. Virtual 4. Override 5. Custom modifiers The following functions do not adhere to this ordering.
[N-22] Function order doesn't follow Solidity style guide
The Solidity style guide states that functions should be laid out in the following order: constructor, receive, fallback, external, public, internal, private. For brevity, only the first function that violates this rule is shown in the following contracts.
Functions with high cyclomatic complexity are harder to understand, test, and maintain. Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns. See here for more information on cyclomatic complexity.
[N-27] Visibility should be explicitly set rather than defaulting to internal
The default visibility for mappings and state variables is internal. Explicitly defining visibility improves readability and reduces margin for error during development, testing and auditing.
[N-28] Imports could be organized more systematically
The contract's interface should be imported first, followed by each of the interfaces it uses, followed by all other files. The contracts below do not follow this layout.
Solidity's style guide states that the maximum suggested line length is 120 characters. Also, if lines exceed 164 characters then a horizontal scroll bar will be required when viewing the file on Github. Extensions such as prettier are a simple solution.
[N-33] Import specific identifiers rather than the whole file
Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file. This improves readability, makes flattened files smaller, and speeds up compilation.
If the length of the arrays are not required to be of the same length, user operations may not be fully executed due to a mismatch in the number of items iterated over, versus the number of items provided in the second array.
When calling a function, use named parameters to improve readability and reduce the chance of making mistakes. For example: _mint({account: msg.sender, amount: _amount})
Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18.
[N-38] Named return variables used before assignment
As no value is written to the variable, the default value is always read. This is usually due to a bug in the code logic that causes an invalid value to be used.
[N-59] Use a struct instead of returning multiple values
Functions that return many variables can become difficult to read and maintain. Using a struct to encapsulate these return values can improve code readability, increase reusability, and reduce the likelihood of errors. Consider refactoring functions that return more than three variables to use a struct instead.
The directive using A for B can be used to attach functions (A) as operators to user-defined value types or as member functions to any type (B). The member functions receive the object they are called on as their first parameter (like the self variable in Python). The operator functions receive operands as parameters. Doing so improves readability, makes debugging easier, and promotes modularity and reusability in the code.
There are 23 instance(s) of this issue:
File: src/ocean/Ocean.sol
18: import { SafeERC20 } from"@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
836: SafeERC20.safeTransferFrom(IERC20(tokenAddress), userAddress, address(this), transferAmount);
875: SafeERC20.safeTransfer(IERC20(tokenAddress), userAddress, transferAmount);
1048: /**1049: * @dev This function determines the correct argument to pass to1050: * the external token contract1051: * @dev Say the in-Ocean unwrap amount (in 18-decimal) is 0.1234567890123456781052: * If the external token uses decimals == 6:1053: * transferAmount == 1234561054: * dust == 7890123456781055: * If the external token uses decimals == 18:1056: * transferAmount == 1234567890123456781057: * dust == 01058: * If the external token uses decimals == 21:1059: * transferAmount == 1234567890123456780001060: * dust == 01061: * @param amount the amount of in-Ocean tokens being unwrapped1062: * @param decimals returned by IERC20(token).decimals()1063: * @return transferAmount the amount passed to SafeERC20.safeTransfer()1064: * @return dust The amount of in-Ocean token that are not unwrapped1065: * due to the mismatch between the external token's decimal basis and the1066: * Ocean's NORMALIZED_DECIMALS basis.1067: */
[N-63] Consider using a timelock for admin/governance functions
Admin functions that change state should consider adding timelocks so that users and other privileged roles can be notified of and react to upcoming changes. Also, this protects users against a compromised/malicious admin account.
There are 1 instance(s) of this issue:
File: src/ocean/Ocean.sol
196: function changeUnwrapFee(uint256nextUnwrapFeeDivisor) externaloverride onlyOwner {
Make sure that functions with a return value always return a valid and assigned value. Even if the default value is as expected, it should be assigned with the default value for code clarity and to reduce confusion.
Unused imports should be removed to improve readability and reduce contract bytecode size. Note that contracts referenced in comments/NatSpec but not utilised by the contract need not be imported.
Starting with version 0.8.4, Solidity has the bytes.concat() function, which allows one to concatenate a list of bytes/strings, without extra padding. Using this function rather than abi.encodePacked() makes the intended operation more clear, leading to less reviewer confusion.
Using delete more closely aligns with the intention of the action, and draws more attention towards the changing of state, which may lead to a more thorough audit of its associated logic"
OpenZeppelin recommends using increaseAllowance and decreaseAllowance instead of approve to mitigate against the problem described here. Source: OpenZeppelin docs, OpenZeppelin ERC20.sol.
[N-79] Use a struct to encapsulate multiple function parameters
If a function has too many parameters, replacing them with a struct can improve code readability and maintainability, increase reusability, and reduce the likelihood of errors when passing the parameters.
[N-80] Use descriptive constant rather than 0 for function arguments
Passing zero as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter). Consider using a constant variable with a descriptive name, so it's clear that the argument is intentionally being used, and for the right reasons.
[N-82] Variable names should not end with an underscore
The use of underscore at the end of the variable name is uncommon and also suggests that the variable name was not completely changed. Consider refactoring variableName_ to variableName.
[N-85] Missing zero check when assigning int/uint to state
There are some missing checks in these functions, and this could lead to unexpected scenarios. Consider always adding a sanity check for state variables.
File: src/ocean/Ocean.sol
6: pragma solidity0.8.20;
8: // OpenZeppelin ERC Interfaces9: import { IERC20Metadata } from"@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
27: import { OceanERC1155 } from"./OceanERC1155.sol";
29: /**30: * @title A public multitoken ledger for defi31: * @author Cowri Labs Team32: * @dev The ocean is designed to interact with contracts that implement IERC20,33: * IERC721, IERC-1155, or IOceanPrimitive.34: * @dev The ocean is three things.35: * 1. At the highest level, it is a defi framework[0]. Users provide a list36: * of interactions, and the ocean executes those interactions. Each37: * interaction involves a call to an external contract. These calls result38: * in updates to the ocean's accounting system.39: * 2. Suporting this defi framework is an accounting system that can transfer,40: * mint, or burn tokens. Each token in the accounting system is identified by41: * its oceanId. Every oceanId is uniquely derived from an external contract42: * address. This external contract is the only contract able to cause mints43: * or burns of this token[1].44: * 3. Supporting this accounting system is an ERC-1155 ledger with all the45: * standard ERC-1155 features. Users and primitives can interact with their46: * tokens using both the defi framework and the ERC-1155 functions.47: *48: * [0] We call it a framework because the ocean calls predefined functions on49: * external contracts at certain points in its exection. The lifecycle is50: * managed by the ocean, while the business logic is managed by external51: * contracts. Conceptually this is quite similar to a typical web framework.52: * [1] For example, when a user wraps an ERC-20 token into the ocean, the53: * framework calls the ERC-20 transfer function, and upon success, mints the54: * wrapped token to the user. In another case, when a user deposits a base55: * token into a liquidity pool to recieve liquidity provider tokens, the56: * framework calls the liquidity pool, tells it how much of the base token it57: * will receive, and asks it how much of the liquidity provider token it58: * would like to mint. When the pool responds, the framework mints this59: * amount to the user.60: *61: * @dev Getting started tips:62: * 1. Check out Interactions.sol63: * 2. Read through the implementation of Ocean.doInteraction(), glossing over64: * the function call to _executeInteraction().65: * 3. Read through the imlementation of Ocean.doMultipleInteractions(), again66: * glossing over the function call to _executeInteraction(). When you67: * encounter calls to LibBalanceDelta, check out their implementations.68: * 4. Read through _executeInteraction() and all the functions it calls.69: * Understand how this is the line separating the accounting for the external70: * contracts and the accounting for the current user.71: * You can read the implementations of the specific interactions in any72: * order, but it might be good to go through them in order of increasing73: * complexity. The called functions, in order of increasing complexity, are:74: * wrapErc721, unwrapErc721, wrapErc1155, unwrapErc1155, computeOutputAmount,75: * computeInputAmount, unwrapErc20, and wrapErc20. When you get to76: * computeOutputAmount, check out IOceanPrimitive, IOceanToken, and the77: * function registerNewTokens() in OceanERC1155.78: */79: contractOceanisIOceanInteractions, IOceanFeeChange, OceanERC1155, IERC721Receiver, IERC1155Receiver {
[N-87] Large or complicated code bases should implement invariant tests
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement invariant fuzzing tests. Invariant fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and invariant fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
[N-89] Codebase should go through formal verification
Formal verification is the act of proving or disproving the correctness of intended algorithms underlying a system with respect to a certain formal specification/property/invariant, using formal methods of mathematics. Some tools that are currently available to perform these tests on smart contracts are SMTChecker and Certora Prover.
M-01 will dispute it since the owner is not an eoa it is a multisig
M-02 adapter only interacting with specific tokens which are safe so can mark this as qa
M-03 we have used 0.8.20 in some other contracts which we have deployed to testnet and haven't faced any issues there so maybe qa again
M-01 will dispute it since the owner is not an eoa it is a multisig
I agree, esp. that now by C4 standards centralization risk is part of the analysis report and not an H/M
M-02 adapter only interacting with specific tokens which are safe so can mark this as qa
I agree, this doesn't seem relevant here
M-03 we have used 0.8.20 in some other contracts which we have deployed to testnet and haven't faced any issues there so maybe qa again
Technically I agree here, this isn't more than a low by C4's standards. Personally, I'd recommend triple-checking that this opcode is indeed not in the compiled contract, or switching to an earlier version.
M-01 will dispute it since the owner is not an eoa it is a multisig
M-02 adapter only interacting with specific tokens which are safe so can mark this as qa
M-03 we have used 0.8.20 in some other contracts which we have deployed to testnet and haven't faced any issues there so maybe qa again