9inch

Smart Contract Security Assessment

Sep 10, 2023

SUMMARY​

ABSTRACT​

Dedaub was commissioned to perform a security audit of the staking pools of the 9inch protocol. The staking pools combine a Masterchef contract for rewards distribution, and staking pools adapted from PancakeSwap staking contracts on BSC.

SETTING & CAVEATS​

The protocol repository is currently private, at 9inchswap/9InchSM. We had previously performed a short audit over the delta of changes of the entire 9inch protocol relative to the underlying forked protocols. The report can be found here. Specifically for the changes to the staking pools, which are over two PancakeSwap public contracts (1,2) on BSC (and not in the PancakeSwap current public repository), we had (in our ealier audit) considered the diff between the original code and commit 0573a9e785d6d971c6f081ca7dd7eccba0052f09 of the 9inch repo. However, several protocol-level considerations were raised and a number of efficiency concerns were mentioned leading to the need to perform this second audit over the changes in the Staking Pools. The current audit was not focused on the delta changes only. We audited the Staking Pools from scratch based on commit e262046798aec021091bb1a839bf4491c98a58d7. Fixes were reviewed at commit e87e2ae8cce6017598290dfff9511b0907313255. (The latter fix commit also includes other changes that do not pertain to this report’s scope.)

The Staking Pools of the previous audit were sharing the same contract for both the pure BBC (i.e. staking token and reward token are both BBC) and non-pure BBC (i.e. staking token is not BBC, while reward is BBC) pools. The changes in the current audit were mainly focused on splitting the pure and the non-pure pools into two separate contracts. Additionally, Flexible and Fixed-Term pools have the same underlying codebase, with the flexible pool contract overriding key parts of the functionality. In total, the auditing effort was over files:

Several of the additions to the PancakeSwap staking contracts implement the ability to stake arbitrary tokens (or rather, more tokens than just the reward token)

Two auditors were commissioned to work on the codebase for 5 working days.

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e., issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e., full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.

PROTOCOL-LEVEL CONSIDERATIONS​

General Pool Codebase Considerations​

Our earlier audit “advise[d] a redesign of the staking part of the protocol” noting that “[w]ith the current underlying contracts, confidence in the correctness of the final implementation will be low.”

We also noted:

Accordingly, the test suite of the project needs to be substantially extended. Right now only basic interactions with the two staking pools are captured in tests. Extensive interactions, under all corner cases, should be fully tested. Both for pureBBC/non-pureBBC cases, all fees should be tested to be computed correctly (and transferred correctly to the appropriate parties), all balances should be checked in complex scenarios involving multiple stakers, all reward rates should be calculated analytically (in a spreadsheet) and the test cases should check that the code computes them correctly, and several conditions should be exercised (e.g., deposit of 0 amount, which is permitted in the code) to ensure that the computations are stable and do not confer an advantage or penalty to the staker.

The latter recommendation (for more extensive testing) was not addressed in the revision considered during this audit. In fact, our testing did reveal serious issues (C1) by following exactly the testing strategy outlined above: trying equivalent stakes to ensure that the computations are stable and do not confer an advantage or penalty to the staker. (The issue turned out to be invalid and caused by other changes, but was not possible to dismiss without a more thorough test suite.)

Upon the final fix review (commit e87e2ae8cce6017598290dfff9511b0907313255), the test suite was extended, lending much more confidence to the validity of the staking operations.

Pools Deployment & Usage Considerations​

There are several considerations when deploying and using the staking pools. We outline them below because they are easy to miss:

• The parent contracts, CakePool and CakeFlexiblePool should only be used with token == bbc, i.e., for pure BBC pools. For non-pure BBC pools, the children contracts, TokenPool and TokenFlexiblePool, should be used instead. (This property is not easily enforceable in the code, since the constructors of the parent contracts are reused in the children.) Otherwise, using a CakePool with token != bbc would result in the users being unable to claim their earned BBC rewards since no claim functionality seems to be present and all transfers are made over the staking tokens only.

• The flexible pool contracts, CakeFlexiblePool and TokenFlexiblePool, should be whitelisted in the non-flexible pool contracts (CakePool and TokenPool correspondingly) so that they do not incur withdrawal fees. This is especially important for TokenFlexiblePool, otherwise, a key computation returns the wrong result. Namely, the following function blindly trusts that the credit that the flexible pool has in the parent pool is equal to the amount it has deposited in it.

  TokenFlexiblePool::balanceOf():205

  function balanceOf() public override view returns (uint256) {
      return totalStakedAmount;
  }
  

  We generally advise revising the above function so that it is less brittle: the balance of a flexible pool in terms of the staking token should be exactly what the parent pool believes it is, not some quantity maintained by the flexible pool alone.

• Users can stake with a zero lock duration directly in the parent (non-flexible, i.e., CakePool) pool instead of using the flexible pool (i.e., CakeFlexiblePool). These two staking approaches are not equivalent but subtly different. Additionally, neither one matches the published PancakeSwap documentation of flexible pools, exhibiting small differences.

  Specifically, the non-flexible pools charge a withdrawal fee when someone withdraws their principal earlier than 3 days from their last deposit, except when the user has been whitelisted inside freeWithdrawFeeUsers.

  CakePool::withdrawOperation():493

  uint256 public withdrawFeePeriod = 72 hours; // 3 days
  
  function withdrawOperation(
      uint256 _shares,
      uint256 _amount
  ) internal virtual {
      ...
      // Calculate withdraw fee
      if (!freeWithdrawFeeUsers[msg.sender] &&
          (block.timestamp < user.lastDepositedTime + withdrawFeePeriod)
      ) {
          uint256 currentWithdrawFee = (currentAmount * withdrawFee) /
              FEE_RATE_SCALE;
          token.safeTransfer(treasury, currentWithdrawFee);
          currentAmount -= currentWithdrawFee;
      }
      token.safeTransfer(msg.sender, currentAmount);
      ...
  }
  

  Per the earlier bullet item, flexible pools should be whitelisted in the parent pools (non-flexible pools) so that they do not incur withdrawal fees which could affect their internal accounting (see TokenFlexiblePool description).

  Notably, the PancakeSwap documentation of flexible pools does not match the behavior in the code. (9inch did not change that logic in the implementation.) The documentation of flexible pools states:

  Unstaking Fee
  0.1% if you unstake (withdraw) within 72 hours.
  Only applies within 3 days of manually staking.
  After 3 days, you can unstake with no fee.
  The 3-day timer resets every time you manually stake more CAKE in the pool.
  This fee only applies to manual unstaking: it does not apply to automatic compounding.
  

  As discussed, flexible pools should not incur a withdrawal fee (and do not in the current PancakeSwap setup either).

  Furthermore, the current configuration of non-flexible pools also incurs no withdrawal fee! The minimum lock duration is 7 days, rendering the withdrawal fee useless, since at withdraw time more than 3 days from staking must have elapsed.

  All the above comments lead to the conclusion that this withdrawal fee becomes redundant as it will not be used at any point. The non-flexible pool users can’t be charged as the minimum lock duration (7 days) exceeds the 3-day period that the fee defines and the flexible pools are whitelisted in the parent pools meaning that they are not charged any withdrawal fees either.

  So, some protocol-level decisions should be made here in order to decide which is the desired behavior and take care of implementing it properly without introducing issues and edge cases like the ones described above.

• Attention is also needed if, in the future, the protocol needs to be refactored and the MIN_LOCK_DURATION changes. Having in mind the above, this constant variable in the non-flexible pools code should not be set to anything less than the value of the withdrawFeePeriod variable. Otherwise, there could be cases where the non-flexible pool stakers would have to pay withdrawal fees when the locking period ends and they attempt to withdraw their principal and rewards.

  For example, assume a MIN_LOCK_DURATION of 2 days and a withdrawFeePeriod of 3 days. Then if someone locks his stake for the minimum possible duration (2 days), then he will lose some of his principal and rewards when withdrawing.

  CakePool::withdrawOperation:495

  TokenPool::withdrawOperation:343

  // Calculate withdraw fee
  if (!freeWithdrawFeeUsers[msg.sender] &&
      (block.timestamp < user.lastDepositedTime + withdrawFeePeriod)
  ) { ... }
  

  The variables of interest that need to be aligned for this to be prevented are the following:

• MAX_WITHDRAW_FEE_PERIOD

• MIN_LOCK_DURATION

• withdrawFeePeriod

  If the code maintains the following condition then the problem would be prevented as the withdrawFeePeriod can be set only up to the MAX_WITHDRAW_FEE_PERIOD value.

  MAX_WITHDRAW_FEE_PERIOD <= MIN_LOCK_DURATION
  

VULNERABILITIES & FUNCTIONAL ISSUES​

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.

CRITICAL SEVERITY​

await track(
    CakeVault.connect(alice), 'deposit', parseEther('10'), 86400 * 7)

await track(
    CakeVault.connect(alice), 'deposit', parseEther('1'), 86400 * 7)
for (let i = 0; i < 9; i++) {
	await track(CakeVault.connect(alice), 'deposit', parseEther('1'), 0)
}

function updateBoostContractInfo(address _user) internal {
    if (boostContract != address(0)) {
        UserInfo storage user = userInfo[_user];
        uint256 lockDuration = user.lockEndTime - user.lockStartTime;
        IBoostContract(boostContract).onCakePoolUpdate(
            _user,
            user.lockedAmount,
            lockDuration,
            totalLockedAmount,
            DURATION_FACTOR
        );
    }
}

HIGH SEVERITY​

if (totalShares != 0) {
    ...
} else {
    currentShares = _amount;
}

bbcPerShare += (claimedAmount * 1 ether) / totalShares;

MEDIUM SEVERITY​

function unlock(
    address _user
) public onlyOperatorOrBBCOwner(_user) whenNotPaused {
    UserInfo storage user = userInfo[_user];
    require(
        user.locked && user.lockEndTime < block.timestamp,
        "Cannot unlock yet"
    );
    depositOperation(0, 0, _user);
}

LOW SEVERITY​

[No low severity issues]

CENTRALIZATION ISSUES​

It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)

function inCaseTokensGetStuck(address _token) public onlyAdmin {
    require(
        _token != address(token),
        "Token cannot be same as deposit token"
    );

    uint256 amount = IERC20(_token).balanceOf(address(this));
    IERC20(_token).safeTransfer(msg.sender, amount);
}

OTHER / ADVISORY ISSUES​

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them to be sure that they have been taken into account and not merely overlooked.

function emergencyWithdraw() public onlyAdmin {
    require(staking, "No staking bbc");
    staking = false;
    parentPool.withdrawAll();
}

    if (_amount > 0) {
        token.safeTransferFrom(_user, address(this), _amount);
        currentAmount = _amount;
    }

    // Calculate lock funds
    if (user.shares > 0 && user.locked) {
        userCurrentLockedBalance = (pool * user.shares) / totalShares;
    ... // many more checks and effects

token.safeTransfer(msg.sender, currentAmount);

if (user.shares > 0) {
    user.lastUserActionAmount =
      (user.shares * balanceOf()) /
      totalShares;
} else {
    user.lastUserActionAmount = 0;
}
user.lastUserActionTime = block.timestamp;

// Dedaub: tokenDecimals should be made immutable to avoid the SLOADs in
//         toEther and fromEther functions
uint8 private tokenDecimals;

function toEther(uint256 _amount) internal view returns(uint256) {
    if(tokenDecimals < 18)
        return _amount * 10 ** (18 - tokenDecimals);
    else if(tokenDecimals > 18)
        return _amount / 10 ** (tokenDecimals - 18);
    return _amount;
}

function fromEther(uint256 _amount) internal view returns(uint256) {
    if(tokenDecimals < 18)
        return _amount / 10 ** (18 - tokenDecimals);
    else if(tokenDecimals > 18)
        return _amount * 10 ** (tokenDecimals - 18);
    return _amount;
}

uint256 currentShare = _shares;
uint256 sharesPercent = (_shares * PRECISION_FACTOR_SHARE) /
        	user.shares;
...  // A
if (_shares == 0 && _amount > 0) {
      ...  // B
} else {
     	currentShare = (sharesPercent * user.shares)/PRECISION_FACTOR_SHARE;
}

uint256 currentShare = _shares;
...  // A
if (_shares == 0 && _amount > 0) {
  	...  // B
}

withdrawAmount = (withdrawAmount * withdrawAmountBooster) / 10000;

DISCLAIMER​

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.

ABOUT DEDAUB​

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.