Skip to content

Instantly share code, notes, and snippets.

@adamdossa

adamdossa/HODLVaultSeviceContract_Security_Review.md

Forked from nachomazzara/Spectre_io_HODLVaultSeviceContract_Security_Review.md
Last active September 13, 2021 11:55
Show Gist options
  • Save adamdossa/d92dbec7062b6b13088004bdd0d456f0 to your computer and use it in GitHub Desktop.
Save adamdossa/d92dbec7062b6b13088004bdd0d456f0 to your computer and use it in GitHub Desktop.
Download ZIP
HODLVaultSeviceContract - Security Review
Raw
HODLVaultSeviceContract_Security_Review.md

FCRACK - Security Review

Table of contest

Introduction

Hivelly team requested the review of the contract under the repository FCRACK referenced by the commit d8e9e36a08b5a9c4b3283f2903b6e0e045033a9f: FCRACKStaking.sol and FCRACKToken.sol.

The rest of the contracts in the repositories are assumed to be audited.

General

H1 - Severe lack of integration and unit testing

The project contains a tests folder with 7 unit tests, 3 for the FCRACKToken contract and 4 for the FCRACKStaking; this test suite gives the project coverage of 57% statements and 36% of branches, deficient for a project composed mainly by a non-standard ERC20.

Some functionality is completely left untested:

  • Token allowances
  • Token transferFrom
  • Token deliver function
  • Token includeInReward function
  • Token excludeFromReward of an address with non-zero balance
  • Token _getCurrentSupply if rOwned sum overflows
  • Token transfer from excluded to excluded addresses
  • Token _transferBothExcluded function
  • Stacking pendingFCRACK function
  • Stacking massUpdatePools function
  • Stacking deposit callbacks
  • Stacking withdraw function
  • Stacking harvest callbacks
  • Stacking withdrawAndHarvest function
  • Stacking emergencyWithdraw function

Missing tests for such core components can lead to unexpected bugs and out-of-spec behaviors; additionally, The developers must extensively test any non-standard token meant to interact with a stacking contract. Tests must cover all edge scenarios, not only the happy paths.

Response

Tests have now been improved to cover the majority of functions.

FCRACKStaking.sol

This contracts seems to be a clone with some changes of the MasterCheckV2 smart contract. The migration is an optional but important piece of code that was removed.

High Severity

H1 - lines 249 and 278 - Rewards ratio inconsistencies

One of the changes related to the original stacking contract is that balances of the LP tokens are not being calculated on demand but stored in poolBalances. If users call withdrawAndHarvest or emergencyWithdraw instead of withdraw, the poolBalances of that LP is not being updated and, therefore the rewards ratio is going to be compromised.

Proposed solution:

  • Update the poolBalance at withdrawAndHarvest and emergencyWithdraw.

Response

Fixed as suggested

FCRACKToken.sol

High Severity

H1 - Unbound iteration on transfer

The ERC20 transfer function makes multiple calls to _getValues, which interns calls _getRate and then _getCurrentSupply. An unbound loop exists on _getCurrentSupply; this loop is called multiple times on each transfer, making the primary functionality of the contract (transfer tokens) inefficient and opening the possibility of leaving the contract unusable. Two possible scenarios could break the token:

    1. Many excluded addresses are added, either by accident or malice.
    1. A future hard fork may increase the cost of accessing storage because the loop reads multiple storage values, the impact of such change would be high.

Those scenarios would make the loop execution too expensive, to the point it could become impossible to make a transfer because transactions couldn't fit on Ethereum's gas limit.

To further aggravate the situation, "includeInReward" the address on the rewards also requires iterating the loop, so in specific scenarios, an honest contract owner couldn't recover from the situation either.

Proposed solution:

  • a. Refactor the balances mechanism and remove all unbounded loops from the contracts.
  • b. (Half-measure) Take an index parameter on includeInReward and validate the index belongs to account, instead of iterating the whole loop.

Notice: Solution b) would still allow a malicious contract owner to freeze all assets related to this token; this could include liquidity on some liquidity pools.

Response

The number of excluded addresses has now been limited to 5. Excluded addresses are meant just for the staking contract, so we could not allow any additional addresses to be excluded after the contract is deployed, but this leaves some additional room to exclude e.g. a DEX in the future if needed.

H2 - Excluded addresses can retroactively recover rewards

When an address is excluded from rewards by using the excludeFromReward, it stops receiving its proportional amount of rewards for all transfers. Instead, those rewards are distributed among all remaining non-excluded holders. However, at any point in time, an address can be re-included in the rewards program. All previous non-assigned rewards will be retroactively assigned, decreasing the balance of all other holders.

A malicious actor with ownership of the contract could use this mechanism to deceive holders and reduce their balances after a period of high activity.

Proposed solutions:

    1. Recompute rBalance without changing rTotal, so the excluded address doesn't receive past rewards.
    1. Don't allow re-including an address on the rewarded addresses set.

Response

The ability to re-include a previously excluded address has been removed.

Medium Severity

M1 - Using contract storage for internal call transport

The FCRACKToken contract defines the variables _taxFee and upfrontFee; these variables are internal and only used to pass information to calculateTaxFee. This information is passed on in the context of a single call, and the values are overwriten on each new transfer. Using contract storage to pass function parameters is a bad design pattern; it substantially increases the cost of making transfers and may cause dangerous bugs if not correctly managed on future project iterations.

Proposed solution:

  • Replace _taxFee and upfrontFee with function parameters on calculateTaxFee, make calculateTaxFee pure.

Response

Code has been refactored to pass these through as function parameters

M2 - Excluded addresses balance may not be fully transferable

When an address is excluded from rewards, balanceOf ignores _rOwned and returns the plain value of _tOwned. However, when sending tokens from the excluded address, both _rOwned and _tOwned are deducted simultaneously. If the sender doesn't have enough _rOwned, the transfer will fail; this can lead to users not being able to transfer their whole balance or contracts becoming stuck, being off by one.

See following test example:

it('Should be able to transfer all balance', async () => {
  await fcrackStaking.grab(signers[4].address, ethers.utils.parseEther('10000'))

  await fcrackToken.connect(signers[4]).deliver(1)
  await fcrackStaking.grab(signers[3].address, 1)

  await fcrackToken.excludeFromReward(signers[3].address)
  await fcrackToken.includeInReward(fcrackStaking.address)

  // Balance is 1! but the user can't transfer it
  const balance = await fcrackToken.balanceOf(signers[3].address)
  expect(balance.toNumber()).to.be.above(0)
  await fcrackToken.connect(signers[3]).transfer(signers[0].address, balance)
})

Response

Deliver function has been removed as it is not required.

M3 - Token freezes if all token holders are excluded

If all token holders are excluded from rewards, tSupply becomes zero, leading to getRate reverting due to div by zero.

Proposed solutions:

    1. Same as L2) mint a small portion of tokens to a burn address, forbid this address from being excluded.

Response

A check has been added when excluding addresses, that the resultant tSupply is not zero.

Low Severity

L1 - Transfer of zero tokens not allowed

The ERC20 standard requires users to be able to transfer 0 tokens. Failure to meet this standard could lead to the token not working correctly with some other contracts.

Proposed solution:

  • Remove require(amount > 0) from the transfer function, instead emit and event and return without side-effects.

Response

Fixed as suggested

L2 - Contract degradation exploit vector

When only a single non-excluded user exists, calling deliver will result in no actual balance decrease, however _rTotal decreases. Such users could call deliver many times and thus decrease _rTotal without effective limits. Reducing _rTotal may result in loss of balance resolution for all subsequent token holders, and because there is no mechanism to increase _rTotal, such loss of resolution would become permanent.

Proposed solution:

  • Mint, a small portion of tokens to a "burn" address on contract creation, forbid this address from being excluded.

Response

Deliver function has been removed as it is not required.

Notes

N1 - line 161 - Incorrect revert message on includeInReward

The includeInReward function reverts with Account is already excluded if the account is excluded, this is a copied message from excludeFromReward; it should say Account is not excluded.

Response

Function has now been removed

Ignacio Mazzara - 08/2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment