SushiSwap Integration - Wave 2 - 0xNodes

Prepared by:

HALBORN

Last Updated 04/26/2024

Date of Engagement by: January 3rd, 2022 - January 10th, 2022

Summary

100% of all REPORTED Findings have been addressed

All findings

Critical

High

Medium

Low

Informational

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details

8.1 Unchecked transfer
8.2 Missing zero address check
8.3 Possible misuse of public functions
8.4 Solc 0.8.4 compiler version contains multiple bugs
8.5 Constant keccak variables are treated as expressions, not constants
8.6 Using ++i consumes less gas than i++ in loops
8.7 Unneeded initialization of uint256 variables to 0
8.8 Uint32/uint64 types are less gas efficient

9. Review Notes
10. Automated Testing

1. INTRODUCTION

0xNodes engaged Halborn to conduct a security audit on their smart contracts beginning on January 3rd, 2021 and ending on January 10th. The security assessment was scoped to the smart contract provided in the Github repository 0xNODES/platform/tree/audit.

2. AUDIT SUMMARY

The team at Halborn was provided a week for the engagement and assigned a full time security engineer to audit the security of the smart contract. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing, smart-contract hacking, and deep knowledge of multiple blockchain protocols.

The purpose of this audit is to:

Ensure that smart contract functions operate as intended
Identify potential security issues with the smart contracts

In summary, Halborn identified some security risks that were acknowledged by 0xNodes team.

3. TEST APPROACH & METHODOLOGY

Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of this audit. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of the bridge code and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the audit:

Research into architecture and purpose
Smart contract manual code review and walkthrough
Graphing out functionality and contract logic/connectivity/functions (solgraph)
Manual assessment of use and safety for the critical Solidity variables and functions in scope to identify any arithmetic related vulnerability classes
Manual testing by custom scripts
Scanning of solidity files for vulnerabilities, security hotspots or bugs. (MythX)
Static Analysis of security for scoped contract, and imported functions. (Slither)
Testnet deployment (Brownie, Remix IDE)

4. SCOPE

IN-SCOPE: The security assessment was scoped to the following smart contracts:

UniswapV3Integration.sol
Kernel.sol
IntegrationMap.sol
Interconnects.sol
YieldManager.sol

Commit ID: 06a3dd2ddcec58f6a32bb479b4f1b79530eec257

5. RISK METHODOLOGY

Vulnerabilities or issues observed by Halborn are ranked based on the risk assessment methodology by measuring the LIKELIHOOD of a security incident and the IMPACT should an incident occur. This framework works for communicating the characteristics and impacts of technology vulnerabilities. The quantitative model ensures repeatable and accurate measurement while enabling users to see the underlying vulnerability characteristics that were used to generate the Risk scores. For every vulnerability, a risk level will be calculated on a scale of 5 to 1 with 5 being the highest likelihood or impact.

RISK SCALE - LIKELIHOOD

5 - Almost certain an incident will occur.
4 - High probability of an incident occurring.
3 - Potential of a security incident in the long term.
2 - Low probability of an incident occurring.
1 - Very unlikely issue will cause an incident.

RISK SCALE - IMPACT

5 - May cause devastating and unrecoverable impact or loss.
4 - May cause a significant level of impact or loss.
3 - May cause a partial impact or loss to many.
2 - May cause temporary impact or loss.
1 - May cause minimal or un-noticeable impact.

The risk level is then calculated using a sum of these two values, creating a value of 10 to 1 with 10 being the highest level of security risk.

Critical

High

Medium

Low

Informational

10 - CRITICAL
9 - 8 - HIGH
7 - 6 - MEDIUM
5 - 4 - LOW
3 - 1 - VERY LOW AND INFORMATIONAL

6. SCOPE

Out-of-Scope: New features/implementations after the remediation commit IDs.

7. Assessment Summary & Findings Overview

Critical

High

Medium

Low

Informational

Impact x Likelihood

Likelihood

Impact

HAL-01

HAL-02

HAL-03

HAL-04

HAL-05

HAL-06

HAL-07

HAL-08

Security analysis	Risk level	Remediation Date
UNCHECKED TRANSFER	Low	Risk Accepted
MISSING ZERO ADDRESS CHECK	Low	Risk Accepted
POSSIBLE MISUSE OF PUBLIC FUNCTIONS	Informational	Acknowledged
SOLC 0.8.4 COMPILER VERSION CONTAINS MULTIPLE BUGS	Informational	Acknowledged
CONSTANT KECCAK VARIABLES ARE TREATED AS EXPRESSIONS, NOT CONSTANTS	Informational	Acknowledged
USING ++I CONSUMES LESS GAS THAN I++ IN LOOPS	Informational	Acknowledged
UNNEEDED INITIALIZATION OF UINT256 VARIABLES TO 0	Informational	Acknowledged
UINT32/UINT64 TYPES ARE LESS GAS EFFICIENT	Informational	Acknowledged

8. Findings & Tech Details

8.1 UNCHECKED TRANSFER

// Low

Description

In the contract UniswapV3Integration the return value of an external transfer call is not checked. Several tokens do not revert in case of failure and return false. If one of these tokens is used, in case of failure, the withdraw() call would not revert and the Kernel contract would not receive any funds.

Code Location

UniswapV3Integration.sol

UniswapV3Integration.sol

// Transfer the funds
IERC20MetadataUpgradeable(token).transfer(
    moduleMap.getModuleAddress(Modules.Kernel),
    transferAmount // Change to account for partial inavailability
);

Score

Impact: 4
Likelihood: 1

Recommendation

RISK ACCEPTED:The 0xNodes team accepted the risk in this issue.

8.2 MISSING ZERO ADDRESS CHECK

// Low

Description

Some initialize functions are missing address validation. Every address should be validated and checked that is different from zero.

UniswapV3Integration.sol

Line 51: function initialize(address[] memory controllers_, address moduleMap_, address nonfungiblePositionManager_, address uniswapFactory_)

Kernel.sol

Line 72: function initialize(address admin_, address owner_, address moduleMap_)
Line 127: function addIntegration(address contractAddress, string memory name)
Line 142: function addToken(address tokenAddress, bool acceptingDeposits, bool acceptingWithdrawals, bool acceptingLping, bool acceptingBridging, uint256 biosRewardWeight, uint256 reserveRatioNumerator, uint256 targetLiquidityRatioNumerator, uint256 transferFeeKValueNumerator, uint256 transferFeePlatformRatioNumerator)
Line 314: function updateTokenRewardWeight(address tokenAddress, uint256 updatedWeight)
Line 330: function updateTokenReserveRatioNumerator(address tokenAddress, uint256 reserveRatioNumerator)
Line 349: function updateTokenTargetLiquidityRatioNumerator(address tokenAddress, uint256 targetLiquidityRatioNumerator)
Line 367: function updateTokenTransferFeeKValueNumerator(address tokenAddress, uint256 transferFeeKValueNumerator)
Line 386: function updateTokenTransferFeePlatformRatioNumerator(address tokenAddress, uint256 transferFeePlatformRatioNumerator)
Line 404: function updateGasAccount(address payable gasAccount)
Line 415: function updateTreasuryAccount(address payable treasuryAccount)
Line 426: function updateGasAccountTargetEthBalance(uint256 gasAccountTargetEthBalance)

IntegrationMap.sol

Line 30: function initialize(address[] memory controllers_, address moduleMap_, address wethTokenAddress_, address biosTokenAddress_)
Line 68: function addIntegration(address contractAddress, string memory name)
Line 145: function addToken(address tokenAddress, bool acceptingDeposits, bool acceptingWithdrawals, bool acceptingLping, bool acceptingBridging, uint256 biosRewardWeight, uint256 reserveRatioNumerator, uint256 targetLiquidityRatioNumerator, uint256 transferFeeKValueNumerator, uint256 transferFeePlatformRatioNumerator)

Interconnects.sol

Line 37: function initialize(address[] memory controllers_, address moduleMap_, address payable relayAccount_)
Line 75: function updateRelayAccount(address payable relayAccount_)

YieldManager.sol

Line 61: function initialize(address[] memory controllers_, address moduleMap_, uint256 gasAccountTargetEthBalance_, uint32 biosBuyBackEthWeight_, uint32 treasuryEthWeight_, uint32 protocolFeeEthWeight_, uint32 rewardsEthWeight_, address payable gasAccount_, address payable treasuryAccount_)
Line 106: function updateGasAccount(address payable gasAccount_)
Line 115: function updateTreasuryAccount(address payable treasuryAccount_)

Score

Impact: 2
Likelihood: 3

Recommendation

RISK ACCEPTED:The 0xNodes team accepted the risk in this issue.

8.3 POSSIBLE MISUSE OF PUBLIC FUNCTIONS

// Informational

Description

In the following contracts there are functions marked as public but they are never directly called within the same contract or in any of their descendants:

UniswapV3Integration.sol

initialize() (UniswapV3Integration.sol#51-62)

Kernel.sol

isManager() (Kernel.sol#644-646)
isOwner(address) (Kernel.sol#650-652)
isLiquidityProvider() (Kernel.sol#656-663)

IntegrationMap.sol

initialize() (IntegrationMap.sol#30-64)

Interconnects.sol

initialize() (Interconnects.sol#37-44)
getRelayAccount() (Interconnects.sol#84-86)
getTokenUserLpBalance() (Interconnects.sol#452-459)
getTokenPoolLpBalance() (Interconnects.sol#462-469)
getTokenUserLpFeeRewardBalance() (Interconnects.sol#490-497)
getTokenLpUsers() (Interconnects.sol#500-507)
getTokenProtocolFeeRewards() (Interconnects.sol#510-517)

YieldManager.sol

initialize() (YieldManager.sol#61-80)
harvestYield() (YieldManager.sol#246-325)
getReserveTokenBalance() (YieldManager.sol#500-515)
getDesiredReserveTokenBalance() (YieldManager.sol#519-540)
getProcessedWethByToken() (YieldManager.sol#577-584)
getProcessedWethByTokenSum() (YieldManager.sol#587-598)
getTokenTotalIntegrationBalance() (YieldManager.sol#602-623)
getGasAccount() (YieldManager.sol#626-628)
getTreasuryAccount() (YieldManager.sol#631-633)
getLastEthRewardsAmount() (YieldManager.sol#636-638)
getGasAccountTargetEthBalance() (YieldManager.sol#641-648)
getEthDistributionWeights() (YieldManager.sol#654-671)

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

8.4 SOLC 0.8.4 COMPILER VERSION CONTAINS MULTIPLE BUGS

// Informational

Description

Solidity compiler version 0.8.9 fixed important bugs in the compiler. The version 0.8.4 is missing this fix:

0.8.9

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

8.5 CONSTANT KECCAK VARIABLES ARE TREATED AS EXPRESSIONS, NOT CONSTANTS

// Informational

Description

In the contract Kernel, the roles are declared the following way:

Kernel.sol

bytes32 public constant OWNER_ROLE = keccak256("owner_role");
bytes32 public constant MANAGER_ROLE = keccak256("manager_role");

Kernel.sol

    bytes32 public constant LIQUIDITY_PROVIDER_ROLE =
        keccak256("liquidity_provider_role");

This results in the keccak256 operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash.

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

8.6 USING ++I CONSUMES LESS GAS THAN I++ IN LOOPS

// Informational

Description

In all the loops, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

UniswapV3Integration.sol

Line 298: for (uint32 i = 1; i <= poolIDCounter; i++)

IntegrationMap.sol

Line 540: for (uint256 tokenId; tokenId < tokenAddresses.length; tokenId++) {

Interconnects.sol

Line 65: for (uint256 i = 0; i < tokens.length; i++) {
Line 111: for (uint256 tokenId; tokenId < tokens.length; tokenId++) {
Line 164: for (uint256 tokenId; tokenId < tokens.length; tokenId++) {
Line 190: lpUserIdx++
Line 226: for (uint256 tokenId; tokenId < tokens.length; tokenId++) {
Line 283: for (uint256 i; i < tokens.length; i++) {
Line 311: for (uint256 i; i < tokens.length; i++) {
Line 329: for (uint256 i; i < tokens.length; i++) {
Line 356: for (uint256 tokenId; tokenId < tokens.length; tokenId++) {
Line 388: lpUserIdx++
Line 425: for (uint256 tokenId; tokenId < tokens.length; tokenId++) {
Line 481: for (uint256 lpUserIdx; lpUserIdx < lpUsers.length; lpUserIdx++) {

YieldManager.sol

Line 133: for (uint256 i = 0; i < deployments.length; i++) {
Line 138: for (uint256 j = 0; j < deployments[i].tokens.length; j++) {
Line 272: tokenIterator++
Line 440: for (uint256 tokenId; tokenId < tokenCount; tokenId++) {
Line 567: for (uint256 tokenId; tokenId < tokenCount; tokenId++) {
Line 593: for (uint256 tokenId; tokenId < tokenAddresses.length; tokenId++) {
Line 617: integrationId++
Line 706: integrationId++

For example, based in the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

8.7 UNNEEDED INITIALIZATION OF UINT256 VARIABLES TO 0

// Informational

Description

uint256 variables are already initialized to 0 by default. uint256 i = 0, for example, reassigns the 0 to i which wastes gas.

Code Location

UniswapV3Integration.sol

Line 107: uint256 amount0Withdrawn = 0;
Line 108: uint256 amount1Withdrawn = 0;
Line 247: uint256 amountA = 0;
Line 248: uint256 amountB = 0;

Interconnects.sol

Line 65: for (uint256 i = 0; i < tokens.length; i++) {
Line 479: uint256 sum = 0;

YieldManager.sol

Line 133: for (uint256 i = 0; i < deployments.length; i++) {
Line 138: for (uint256 j = 0; j < deployments[i].tokens.length; j++) {

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

8.8 UINT32/UINT64 TYPES ARE LESS GAS EFFICIENT

// Informational

Description

uint32, uint64... variables are less gas efficient than uint256. Due to how the EVM natively works on 256-bit numbers, using for example 32-bit number introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

This happens in multiple functions across the different smart contracts:

UniswapV3Integration.sol

Line 37: uint32 public override poolIDCounter;
Line 40: mapping(uint32 => PositionNFT) internal pools;
Line 43: mapping(uint32 => mapping(address => uint256)) public override balances;
Line 64: function _verifyPoolAndTokens(address token, uint32 poolID) internal view {
Line 72: modifier verifyPoolAndTokens(address token, uint32 poolID) {
Line 81: uint32 poolID
Line 99: uint32 poolID
Line 234: function deploy(uint32 poolID) external override {
Line 239: uint32 poolID,
Line 298: for (uint32 i = 1; i <= poolIDCounter; i++) {
Line 317: function getPoolBalance(uint32 poolID)
Line 337: function getPool(uint32 poolID)
Line 391: function getPendingYield(uint32 poolID)

Kernel.sol

Line 103: function setBiosRewardsDuration(uint32 biosRewardsDuration)
Line 223: uint32 biosBuyBackEthWeight,
Line 224: uint32 treasuryEthWeight,
Line 225: uint32 protocolFeeEthWeight,
Line 226: uint32 rewardsEthWeight

IntegrationMap.sol

Line 15: uint32 private constant RESERVE_RATIO_DENOMINATOR = 1_000_000;
Line 26: uint32 private constant TARGET_LIQUIDITY_RATIO_DENOMINATOR = 1_000_000;
Line 27: uint32 private constant TRANSFER_FEE_K_VALUE_DENOMINATOR = 1_000_000;
Line 28: uint32 private constant TRANSFER_FEE_PLATFORM_RATIO_DENOMINATOR = 1_000_000;
Line 669: returns (uint32)
Line 694: returns (uint32)
Line 719: returns (uint32)
Line 744: returns (uint32)

Interconnects.sol

Line 416: uint32 targetLiquidityRatioDenominator = integrationMap
Line 418: uint32 kValueDenominator = integrationMap
Line 420: uint32 protocolFeeDenominator = integrationMap

YieldManager.sol

Line 32: uint32 private biosBuyBackEthWeight;
Line 33: uint32 private treasuryEthWeight;
Line 34: uint32 private protocolFeeEthWeight;
Line 35: uint32 private rewardsEthWeight;
Line 65: uint32 biosBuyBackEthWeight_,
Line 66: uint32 treasuryEthWeight_,
Line 67: uint32 protocolFeeEthWeight_,
Line 68: uint32 rewardsEthWeight_,
Line 94: uint32 biosBuyBackEthWeight_,
Line 95: uint32 treasuryEthWeight_,
Line 96: uint32 protocolFeeEthWeight_,
Line 97: uint32 rewardsEthWeight_
Line 547: returns (uint32 ethWeightSum)
Line 659: uint32,
Line 660: uint32,
Line 661: uint32,
Line 662: uint32,

In general, the usage of these smaller types only improves the gas costs in cases where the variables can be packed together, for example structs.

Score

Impact: 1
Likelihood: 1

Recommendation

ACKNOWLEDGED:The 0xNodes team acknowledged this issue.

9. Review Notes

CONTRACT INITIALIZATION

No major issues found in the initialization, although, it is recommended to initialize ReentrancyGuardUpgradeable in the Interconnects contract by calling __ReentrancyGuard_init().

10. Automated Testing

STATIC ANALYSIS REPORT

Description

Halborn used automated testing techniques to enhance the coverage of certain areas of the scoped contracts. Among the tools used was Slither, a Solidity static analysis framework. After Halborn verified all the contracts in the repository and was able to compile them correctly into their abi and binary formats, Slither was run on the all-scoped contracts. This tool can statically verify mathematical relationships between Solidity variables to detect invalid or inconsistent usage of the contracts' APIs across the entire code-base.

Slither results

Slither only found some issues in the following smart contracts:

Kernel.sol

IntegrationMap.sol

Interconnects.sol

YieldManager.sol

No major issues were found by Slither.

Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.

1. Introduction
2. Audit summary
3. Test approach & methodology
4. Scope
5. Risk methodology
6. Scope
7. Assessment summary & findings overview
8. Findings & Tech Details

8.1 Unchecked transfer
8.2 Missing zero address check
8.3 Possible misuse of public functions
8.4 Solc 0.8.4 compiler version contains multiple bugs
8.5 Constant keccak variables are treated as expressions, not constants
8.6 Using ++i consumes less gas than i++ in loops
8.7 Unneeded initialization of uint256 variables to 0
8.8 Uint32/uint64 types are less gas efficient

9. Review Notes
10. Automated Testing

// Download the full report

* Use Google Chrome for best results

** Check "Background Graphics" in the print settings if needed