9inch
Smart Contract Security Assessment
July 11, 2023
SUMMARY
ABSTRACT
Dedaub was commissioned to perform a security audit of the 9inch protocol. The protocol is a combination of a Uniswap v2 adaptation, a Masterchef contract, and staking pools adapted from PancakeSwap on BSC.
The main body of the protocol consists of simple changes over Uniswap v2 and appears highly secure and functional. However, we are highly concerned about the staking pools adaptation. The code seems not-yet-mature and the changes are error-prone.
SETTING & CAVEATS
The protocol repository is currently private, at 9inchswap/9InchSM. The audit scope is the delta of changes relative to the underlying forked protocols. The scope was fully defined by the client, in an in-repo PDF file detailing all edits. Especially 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 considered the diff between the original code and commit 0573a9e785d6d971c6f081ca7dd7eccba0052f09 of the 9inch repo.
The additions to the two 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 3 working days. Due to complexity in the staking contracts (esp. in the original code and not in the in-scope edits), the audit extended over a full week.
As part of the audit, we also reviewed the fixes for the issues included in the report. The fixes were delivered at commit 6c662e902f0c26703b8b11189234c06b3732d793. This commit also included several more changes which have not been inspected as they did not pertain to this report’s items. Even though we found that the changes have been properly implemented and address the issues listed in the report (the ones marked as RESOLVED), we again want to emphasize and point out our reservations about the underlying code used for the token pools (see the PROTOCOL-LEVEL CONSIDERATIONS section for more). Several of the resolved issues that describe gas inefficiencies, were meant to be taken only as examples. The entire logic of the TokenPool and TokenFlexiblePool contracts should be revisited and refactored in order to be able to be deployed on the Ethereum mainnet, as the team intends.
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
The base protocol (a Uniswap v2 clone) is solid and the changes in the fork appear to be trustworthy. The same is true of the Sushiswap MasterChef adaptation. However, we are quite concerned about the two staking pool contracts adapted from PancakeSwap on BSC. Specifically:
- The original code for these contracts is of relatively bad quality, as evidenced by dead code, repeat assignments, convoluted logic, lack of invariants;
- The bad quality of the original code is reflected in the error-proneness of the delta implemented by 9inch. We identify several serious issues;
- The original code for these contracts is extremely gas-inefficient and therefore a bad fit for Ethereum mainnet deployment.
We therefore advise a redesign of the staking part of the protocol. With the current underlying contracts, confidence in the correctness of the final implementation will be low.
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.
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:
- User or system funds can be lost when third-party systems misbehave.
- DoS, under specific conditions.
- Part of the functionality becomes unusable due to a programming error.
- Breaking important system invariants but without apparent consequences.
- Buggy functionality for trusted users where a workaround exists.
- Security issues which may manifest when the system evolves.
Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
[No critical severity issues]
HIGH SEVERITY
Dismissed
pool not to be used with tokens that allow reentrancy on transfers
In the TokenFlexiblePool contract, the withdraw function has been modified to support custom ERC20 tokens as staking tokens, as opposed to the PancakeSwap code which only supports CAKE as a staking and reward token. Additionally, none of the functions have re-entrancy guards.
Therefore, if any of the custom ERC20 tokens contain transfer hooks (the worst offender being ERC 777 tokens), this would allow an attacker to break into the system and drain all the BBC reward tokens from the pool. If deployment is careful to only use tokens with no callbacks on-transfer, the code should be safe.
Specifically, the function calculates the user's accumulated earnings and updates their records before performing the withdrawal. However, it also doesn't follow the Checks-Effects-Interactions model to prevent re-entrancy attacks and updates the important storage variables after transferring the staking tokens back to the user allowing them to re-enter the system and reuse their outdated records.
TokenFlexiblePool::withdraw():268
function withdraw(uint256 _shares) public {
UserInfo storage user = userInfo[msg.sender];
require(_shares > 0, "Nothing to withdraw");
require(_shares <= user.shares, "Withdraw amount exceeds balance");
uint256 currentPerformanceFee;
uint256 performanceFeeShares;
bool _isBBCPool = isPureBBCPool();
if(_isBBCPool) {
...
} else {
uint256 claimedAmount = bbcPool.claim();
bbcPerShare += claimedAmount * 1 ether / totalShares;
// Dedaub: earnAmount holds the user's accumulated rewards based
// on their total user.shares and subtracting the
// userRewardDebt value
uint256 earnAmount =
bbcPerShare * user.shares / 1 ether
- userRewardDebt[msg.sender];
if(user.shares > 0) {
// Dedaub: Here the accumulated rewards are added to the total
// rewards that the user should receive
userRewardPending[msg.sender] += earnAmount;
}
...
}
...
currentAmount =
available() >= currentAmount ? currentAmount : available();
// Dedaub: This token transfer can allow an attacker re-enter the
// function if the token has ERC777-like transfer hooks
token.safeTransfer(msg.sender, currentAmount);
// Dedaub: The withdrawn user's shares are deducted from the user's
// records after the token transfer allowing them to re-enter
// and reuse the entire amount of their shares even if some
// of them have already been used
user.shares -= _shares;
totalShares -= _shares;
if (user.shares > 0) {
user.bbcAtLastUserAction =
(user.shares * balanceOf()) / totalShares;
} else {
user.bbcAtLastUserAction = 0;
// Dedaub: When the user has no other shares in the system,
// receives all accumulated rewards
bbc.safeTransfer(msg.sender, userRewardPending[msg.sender]);
userRewardPending[msg.sender] = 0;
}
if(!_isBBCPool) {
// Dedaub: userRewardDebt also gets updated after the transfer
userRewardDebt[msg.sender] = bbcPerShare * user.shares / 1 ether;
}
...
}
This would allow an attacker to enter the system, obtain a large number of shares, and then request to withdraw 1 share at a time re-entering on token transfer with another portion of their shares. This allows them to arbitrarily multiply their pending rewards by the number of the re-entrancies they perform. This happens because their records weren't updated before they received the tokens, and the system calculates the rewards earned based on the total number of the user’s shares.
For example, if someone has 101 shares, they can attack the protocol by re-entering the function 100 times withdrawing 1 share at a time, making x100 their original reward and effectively draining all BBC reward tokens held by the Pool.
This problem can be addressed either by properly implementing the Checks-Effects-Interactions model, which consists of updating all important storage variables before making external calls to potentially untrusted parties, or by adding re-entrancy guards to the contract function to prevent such scenarios.
Similarly, we highly recommend applying such security measures to the TokenPool contract as well, which contains similar logic in several parts and also interacts with the custom ERC20 tokens.
In TokenFlexiblePool, when a custom ERC20 is used as the staking token, the withdraw function calculates the user's accumulated rewards based on their shares and the value of the userRewardDebt. It then charges a performance fee, deducted from this accumulated amount, but it then mistakenly subtracts the entire amount earned from their records, causing them to lose that portion of reward tokens.
The users can get their rewards either by claiming them directly using claim, or when withdrawing all their shares. Hence, they can end up getting less than they were supposed to.
TokenFlexiblePool::withdraw():222
function withdraw(uint256 _shares) public {
UserInfo storage user = userInfo[msg.sender];
require(_shares > 0, "Nothing to withdraw");
require(_shares <= user.shares, "Withdraw amount exceeds balance");
uint256 currentPerformanceFee;
uint256 performanceFeeShares;
bool _isBBCPool = isPureBBCPool();
if(_isBBCPool) {
...
} else {
uint256 claimedAmount = bbcPool.claim();
bbcPerShare += claimedAmount * 1 ether / totalShares;
// Dedaub: earnAmount holds the user's accumulated rewards based
// on their total user.shares and subtracting the
// userRewardDebt value
uint256 earnAmount =
bbcPerShare * user.shares / 1 ether
- userRewardDebt[msg.sender];
if(user.shares > 0) {
// Dedaub: Here the accumulated rewards are added to the total
// rewards that the user should receive
userRewardPending[msg.sender] += earnAmount;
}
if(earnAmount > 0) {
currentPerformanceFee =
(earnAmount * performanceFee) / 10000;
payFee(currentPerformanceFee);
emit ChargePerformanceFee(
msg.sender, currentPerformanceFee, performanceFeeShares);
// Dedaub: Deduction of the entire earnAmount instead of just
// the fee paid (currentPerformanceFee)
userRewardPending[msg.sender] -= earnAmount;
}
}
...
if (user.shares > 0) {
...
} else {
user.bbcAtLastUserAction = 0;
// Dedaub: When the user has no other shares in the system,
// receives all accumulated rewards
bbc.safeTransfer(msg.sender, userRewardPending[msg.sender]);
userRewardPending[msg.sender] = 0;
}
...
}
Therefore, instead of subtracting earnAmount from the userRewardPending value, the currentPerformanceFee should be deducted so that users do not lose their rewards.
In TokenPool, when a custom ERC20 is used as the staking token, users can claim their rewards through the claim function, which updates their records to include the rewards accumulated since their last interaction with the system, charging a performance fee on top of them. However, the deducted fees are never sent to the treasury and, given the resetting of any variables that held this information, the charged fees are lost.
TokenPool::claim()
function claim() public returns (uint256) {
if(!isPureBBCPool()) {
UserInfo storage user = userInfo[msg.sender];
if(!user.locked) {
uint256 harvestedAmount = harvest();
bbcPerShare += harvestedAmount * 1 ether / totalShares;
// Dedaub: The accumulated rewards are added to
// userRewardPending
userRewardPending[msg.sender] +=
bbcPerShare * user.shares / 1 ether
- userRewardDebt[msg.sender];
uint256 currentBBCAmount = userRewardPending[msg.sender];
if(currentBBCAmount > 0) {
if (!freePerformanceFeeUsers[msg.sender]) {
uint256 feeRate = performanceFee;
if (_isContract(msg.sender)) {
feeRate = performanceFeeContract;
}
// Dedaub: The calculated fee is deducted from the
// total amount of the user's rewards
currentBBCAmount -=
currentBBCAmount * feeRate / 10000;
}
bbc.safeTransfer(msg.sender, currentBBCAmount);
// Dedaub: The only variable that could be used to
// calculate the charged fee at a later point gets
// cleared here causing the fees to be lost
userRewardPending[msg.sender] = 0;
}
userRewardDebt[msg.sender] =
bbcPerShare * user.shares / 1 ether;
return currentBBCAmount;
}
}
return 0;
}
A logic similar to the one used in TokenFlexiblePool::payFee could be added to directly transfer the charged fees to treasury on each claim operation to ensure that no fees are lost.
MEDIUM SEVERITY
Dismissed
pool only to be used with tokens that have 18 decimals
There is no adjustment of decimals anywhere in the staking contracts (TokenPool, TokenFlexiblePool). Therefore, the code cannot handle any of a variety of tokens that have fewer (or, rarely, more) than 18 decimals.
LOW SEVERITY
The following storage assignment in TokenPool is unused, since the storage location will be overwritten before it is read. (This is part of general gas-inefficiency of the original PancakeSwap staking contracts.)
TokenPool::updateUserShare():224
if(isPureBBCPool()) {
uint256 totalAmount = (user.shares * balanceOf()) / totalShares;
totalShares -= user.shares;
user.shares = 0; // Dedaub: dead code
uint256 earnAmount = totalAmount - user.bbcAtLastUserAction;
uint256 currentPerformanceFee = (earnAmount * feeRate) / 10000;
if (currentPerformanceFee > 0) {
bbc.safeTransfer(treasury, currentPerformanceFee);
totalAmount -= currentPerformanceFee;
}
// Recalculate the user's share.
uint256 pool = balanceOf();
uint256 newShares;
if (totalShares != 0) {
newShares = (totalAmount * totalShares) / (pool - totalAmount);
} else {
newShares = totalAmount;
}
user.shares = newShares; // Dedaub: assigned here, not read in between
totalShares += newShares;
} else { … }
The PancakeSwap staking contracts have not been written with deployment to the Ethereum mainnet in mind, and therefore appear quite gas-inefficient. There are many examples, including in the code fragment of issue L1:
TokenPool::updateUserShare():221
if(isPureBBCPool()) {
uint256 totalAmount = (user.shares * balanceOf()) / totalShares;
totalShares -= user.shares;
// Dedaub: user.shares SLOADed twice. This is an expensive operation
...
if (totalShares != 0) {
newShares = (totalAmount * totalShares) / (pool - totalAmount);
// Dedaub: totalShares SLOADed twice in a row
} else {
newShares = totalAmount;
}
user.shares = newShares;
totalShares += newShares;
// Dedaub: and a third SLOAD, before the SSTORE
} else { … }
Another instance is:
TokenPool::depositOperation():289
UserInfo storage user = userInfo[_user];
...
if (user.lockEndTime >= block.timestamp) { … }
totalLockDuration += user.lockEndTime - user.lockStartTime;
// Dedaub: SLOAD user.lockEndTime again
As well as:
TokenPool::withdrawOperation():456
if(!isPureBBCPool()) {
if(user.shares==0 && userRewardPending[msg.sender] > 0) {
bbc.safeTransfer(msg.sender, userRewardPending[msg.sender]);
// Dedaub: userRewardPending[msg.sender] is repeated, does two SHA3s
// and two SLOADS
And:
TokenFlexiblePool::claim():303
userRewardPending[msg.sender] += bbcPerShare * user.shares / 1 ether -
userRewardDebt[msg.sender];
uint256 earnAmount = userRewardPending[msg.sender];
// Dedaub: Storing the value in a local variable by reading it from
// storage. If the two statements are inverted (to store the
// complex expression locally and then write it to storage),
// the SLOAD can be avoided.
OTHER / ADVISORY ISSUES
This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.
The original PancakeSwap staking pools code contained checks for the minimum amounts withdrawn or deposited. It is unclear why these have been removed–possibly because absolute amounts do not make sense for different tokens? However, it is possible that the checks provided some numeric stability in the original code.
The following calculation seems to only be introducing numerical inaccuracy, with no real benefit:
TokenPool::withdrawOperation():428
uint256 sharesPercent = (_shares * PRECISION_FACTOR_SHARE) / user.shares;
...
currentShare = (sharesPercent * user.shares) / PRECISION_FACTOR_SHARE;
// Dedaub: sharesPercent is not used anywhere else, ends up being
// pointless, since currentShare ends up being just _shares
The code of TokenPool and TokenFlexiblePool can only be understood to be correct under several invariants that are never stated. These include at least:
- storage variable bbcAtLastUserAction (in TokenPool) is only read when isPureBBCPool is true
- function TokenPool::getPricePerFullShare has a meaningful definition only under condition isPureBBCPool, and that’s the only case when the function is called inside the smart contracts (in TokenFlexiblePool)
- local variable chargeFeeFromDeposite (in TokenFlexiblePool::deposit) implies isPureBBCPool.
Making such conditions explicit (in comments and code structure) is important for reasoning about the correctness of the code.
The two conditionals below can be easily merged:
TokenFlexiblePool::deposit():121 & 124
if (currentPerformanceFee > 0) {
performanceFeeShares =
(currentPerformanceFee * totalShares) / balanceOf();
}
if (currentPerformanceFee > 0) { ... }
The code below can be simplified from:
TokenPool::depositOperation():305 & 311
if (totalShares == 0) {
...
}
if(!isPureBBCPool() && totalShares > 0) { ... }
to:
if (totalShares == 0) {
...
}
else if (!isPureBBCPool()) { ... }
Our recommendation is for all numeric constants to be given a symbolic name at the top of the contract, instead of being interspersed in the code. E.g.,
TokenPool::updateUserShare()
uint256 currentPerformanceFee = (earnAmount * feeRate) / 10000;
// Dedaub: give name to constant? E.g., "FEE_RATE_SCALE"?
The same constant (10000) appears in several locations in the code.
Different contracts are compiled with different solidity versions. More specifically, versions from 0.4.x to 0.8.12 are being used in several different contracts. For deployment, we recommend no floating pragmas and the use of a specific version across the protocol to be confident about the baseline guarantees offered by the compiler. Versions below 0.8.17, at the time of writing, have some known bugs.
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.