Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

The feeHandler has a privileged role with respect to control of rewards for the DAO and Treasury. 

Centralization Risk in feeHandler.sol

Users are at risk of losing rewards depending on order of functions called with respect to `deposit()`, `withdraw()` and `harvest()`.

For example, suppose that we have three sets of epochs: Epoch 1-10; Epoch 11-20; Epoch 21-30. Furthermore, suppose that the respective `accTokenPerShareAtEpoch` is 3, 2, and 2 during the Epoch sets. Let Bob deposits 100 LP tokens using deposit at Epoch 1, 11 and 21 all into the same pool. 

During Epoch 1-10, the function `pendingRewardsInternal` computes Bob's reward as 300 tokens. During Epoch 11-20, the function `pendingRewardsInternal` computes Bob's reward as 400 tokens. Then during Epoch 21-30, the function `pendingRewardsInternal` computes Bob's reward as 600 tokens.

If `harvest` is called before each `deposit` then Bob will receive all of his rewards. However, if `harvest` is called after the third `deposit` then Bob will only receive 1000 tokens. Since `harvest` computes the reward as _pendingRewardToken plus user.rewardDebt. In this case, `_pendingRewardToken`  would be 600 tokens since the reward is computed from epoch 21-30. While the `user.rewardDebt` is defined by epochs 11-20.

This issue is also found in the Vault contract for BeetsFi and SpiritBoostedFarms.


Loss of User Rewards in vault.sol

In the contract `liquidVaultV1` and `LinSpiritVault`, the role `_owner` has authority over the functions `setPerformanceFee`; `add`; `set`; `setRewardPerSecond`; `setDistributionRate`; `setFeeAddress`. 

If there is a compromise to the `_owner` account then the vault system rewards and fees can be redirected to the attackers wallet. For example in `liquidVaultV1`, a compromised owner would be able to set the fees to 20 percent and change the fee address to their own. 


This issue is also found in the Vault and VaultV2 contract for BeetsFi and SpiritBoostedFarms.


Centralization Risk in vault.sol

In the contract `StrategyLinSpiritFarm` the role `governance` has authority over all of the functions: `whitelistHarvesters`; `revokeHarvesters`; `setGovernance`; `setDepositor`; `withdrawAll`.

If the governance role is compromised then the attacker would be able to withdraw and control all LP tokens associated with the strategy contract. 

This issue is also found in the `StrategyBeetsFarm` and `StrategyGeneralBoostLinSpirit`.



Centralization Risk in StrategyGeneralBoostLinSpirit.sol

The function `withdraw(IERC20)` has both the modifier onlyBenevolent and checks if the sender is governance. Therefore the condition is checked twice. 

Withdraw Double Checks Require Condition

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.

Variables That Could Be Declared as Immutable

The linked variables could be declared as `constant` since these state variables are never modified.

Variables That Could Be Declared as `constant`

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to different compiler versions. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.



Missing Error Messages

In the contract `NonIncentivizedChef` the role `_owner` has authority over setting strategies. A compromised strategy is able to drains users Token/Lp positions. 

LiquidDriver

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

11

0

3

4

4