Horsea Finance

CertiK10 Mar 2023Code version: not recorded (see the report)

Audit

CertiK reviewed HorseaFi (Horsea Finance) on 10 Mar 2023: 28 findings, 0 critical; 2 acknowledged, 26 open.

No open critical findings

Open the report Open HorseaFi’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings28

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged
Critical	0	0
High	0	1
Medium	0	1
Low	26	0

Low`claimStuckTokens()` Can Claim Reward TokensOpen

The function `claimStuckTokens()` can claim any token from the dividend tracker, including the `rewardToken`. If these tokens are claimed accidentally, then there will not be enough tokens in the contract to pay of the stored dividends.

Low`public` used instead of `external`Open

The functions linked in the lines above are `public`, but they are never called within the contract and can instead be `external`.

Low`walletToWalletTransferWithoutFee` Is UnnecessaryOpen

`walletToWalletTransferWithoutFee` is always `true` and thus is not needed to be checked against.

LowCannot Unexclude From DividendsOpen

If an address is mistakingly excluded from dividends, there is no way to reverse this action.

LowCatch Not Properly HandledOpen

The catches in the try-catch patterns linked above are empty.

LowCheck Effect Interaction Pattern ViolatedOpen

The order of external call/transfer and storage manipulation must follow the check-effect-interaction pattern.

LowDividend Trackers Can Be Updated When There Are DOpen

The functions `updateDividendTracker()` and `updateDividendTrackerTwo()` change the dividend trackers, but can be called to change the dividend tracker when there are dividends left in the contract to withdraw. As the only way for users to withdraw these fund is to interact through the `HoreseaFi` contract, this will lock users unclaimed dividends in the contract.

LowFees Adjusted Before Balance Reaches `swapTokensAOpen

When tokens are bought or sold, a percentage of the tokens is taken as a fee, with that fee allocated to liquidity, marketing, rewards, or buyback. If the fees are changed after fees have started to accumulate in the contract and the contracts balance has not yet reached `swapTokensAtAmount`, then the tokens held by the contract will be allocated according to the new tax percentages.

LowIncorrect Error MessageOpen

In the function `claimStuckTokens()`, if the input `token` is `address(this)`, then it returns the error message "Owner cannot claim native tokens". However, the word native tokens implies the token of the blockchain and may cause confusion. We recommend changing the error message to "Owner cannot claim HORSEA".

LowMissing Zero Address ValidationOpen

Addresses should be checked before assignment or external call to make sure they are not zero addresses. ```solidity=575 rewardToken = _rewardToken; ``` - `_rewardToken` is not zero-checked before being used. --- ```solidity=696 payable(receiver).transfer(address(this).balance); ``` - `receiver` is not zero-checked before being used. --- ```solidity=1036 uniswapV2Pair = _uniswapV2Pair; ``` - `_uniswapV2Pair` is not zero-checked before being used.

Show 10 more

LowNew `uniswapV2Pair` May Not Be Set As An AutomateOpen

If `updateUniswapV2Router()` is called, then the new `uniswapV2Pair` will not be set as an automated market maker pair. As this pair is set as the `uniswapV2Pair`, it will not be possible to set it as an automated market maker pair due to the check in the function `setAutomatedMarketMakerPair`.

LowRaising Minimum Token Balance For Dividends DoesOpen

For a user to receive dividends, their token balance should exceed the `minimumTokenBalanceForDividends` variable. This variable can be changed via the `updateMinimumTokenBalanceForDividends()` function. However, when `minimumTokenBalanceForDividends` is changed, the users that can receive dividends is unchanged. This means that if `minimumTokenBalanceForDividends` is increased, users whose balance is below the new `minimumTokenBalanceForDividends` but above the old `minimumTokenBalanceForDividends` can still receive dividends.

LowRedundant CheckOpen

In the function `_transfer()`, it is checked that both `contractTokenBalance > 0 && bnbShare > 0`. However, it can just be checked that `contractTokenBalance > 0`. This is because if `contractTokenBalance > 0`, then this means that `liquidityTokens < contractTokenBalance` so that the `liquidityShare` is not the `totalShare`. This then necessarily means that the `bnbShare` is greater than `0` as the `liquidityShare + bnbShare = totalShare`.

LowRouter And Pair May Not Be Excluded From DividendOpen

In the `constructor()`, the `uniswapV2Pair` is never excluded from dividends. In addition, in the function `updateUniswapV2Router()`, it is never checked that the new router and pair are excluded from dividends and excluded if they are not already.

LowState Variable Should Be Declared ConstantOpen

State variables that never change should be declared as `constant` to save gas. --- ```solidity=924 address private DEAD = 0x000000000000000000000000000000000000dEaD; ``` - `DEAD` should be declared `constant`.

LowThird Party DependencyOpen

The contract is serving as the underlying entity to interact with PancakeSwap and reward tokens. The scope of the audit treats third party entities as black boxes and assume their functional correctness. However, in the real world, third parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of third parties can possibly create severe impacts, such as increasing fees of third parties, migrating to new LP pools, etc.

LowTransfers Are Disabled With No Error MessageOpen

In the contract `DividendPayingToken`, transfers are disabled by including the following check in the `_transfer()` function: ```solidity=636 require(false); ``` However, no error message is returned which may cause confusion.

LowUnnecessary `Payable` CastingOpen

The `payable` casting is only needed when BNB is to be sent to the address, The lines referenced above only transfer ERC20 tokens making the `payable` casting unnecessary:

LowUnnecessary CheckOpen

In the function `getAccount`, the variable `processesUntilEndOfArry` is supposed to store the number of indexes left until the end of the array. The check that `tokenHoldersMap.keys.length > lastProcessedIndex` will always pass as the largest an index can be is `tokenHoldersMap.keys.length - 1`.

LowUnnecessary CodeOpen

In the contract `DividendPayingToken`, transfers are disabled via a check in the `_transfer()` function. The code after this check in the `_transfer()` function will never be reached and can be removed.

Show 6 more

LowUnnecessary Use of SafeMathOpen

The `SafeMath` library is used unnecessarily. With Solidity compiler versions 0.8.0 or newer, arithmetic operations will automatically revert in case of integer overflow or underflow.

LowUnsafe Integer CastOpen

The function `getIndexOfKey()` returns a `uint` that is cast as an `int` without evaluating its bounds. Similarly in the function `getAccount()`, both `lastProcessedIndex` and `processesUntilEndOfArray` are `uint256` that are cast as `int256` without evaluating their bounds. Considering these scenarios are unlikely we have given this an informational severity.

LowUnused Return ValueOpen

The functions `claim()`, `claimAddress()`, `claimTwo()`, and `claimAddressTwo()` all call `processAccount()` from a dividend tracker contract, but they never check the return value. As this function returns false if the calculated amount is `0` it can be used to provide an error or event to the user that no dividends were withdrawn.

LowUsage of `transfer`/`send` for sending EtherOpen

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring Ether, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, EVM instructions' gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation. ```solidity=696 payable(receiver).transfer(address(this).balance); ``` - `DividendTracker.claimStuckTokens` uses `transfer()`. --- ```solidity=1010 payable(msg.sender).transfer(address(this).balance); ``` - `HorseaFi.claimStuckTokens` uses `transfer()`.

LowUser-Defined GettersOpen

The linked functions are equivalent to the compiler-generated getter functions for the respective variables.

LowWithdrawDividend Does Not Revert On FailureOpen

If the reward token returns false instead of reverting, then this transaction will succeed, but will instead undo the update to `withdrawnDividends[user]`. In addition, the token may not faithfully adhere to the standard and return false when it transfers only a partial amount. This would cause the stored amount of `withdrawnDividends[user]` to be incorrect.