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

The contracts `Strategy`, `StrategyMars` and `StrategyPCS` swap tokens and add liquidity to swapping pools. For example, in `Strategy`:
```solidity=494
    function _safeSwap(
        address _uniRouterAddress,
        uint256 _amountIn,
        uint256 _slippageFactor,
        address[] memory _path,
        address _to,
        uint256 _deadline
    ) internal virtual {
        uint256[] memory amounts =
            IPancakeRouter02(_uniRouterAddress).getAmountsOut(_amountIn, _path);
        uint256 amountOut = amounts[amounts.length.sub(1)];

        IPancakeRouter02(_uniRouterAddress)
            .swapExactTokensForTokensSupportingFeeOnTransferTokens(
            _amountIn,
            amountOut.mul(_slippageFactor).div(1000),
            _path,
            _to,
            _deadline
        );
    }
```
In `StrategyMars`:
```solidity=173
                IPancakeRouter02(marsRouterAddress).addLiquidity(
                    token0Address,
                    token1Address,
                    token0Amt,
                    token1Amt,
                    0,
                    0,
                    address(this),
                    block.timestamp.add(600)
                );
```

A sandwich attack might happen when an attacker observes a transaction swapping tokens or adding liquidity without setting restrictions on slippage or minimum output amount. The attacker can manipulate the exchange rate by frontrunning (before the transaction is attacked) a transaction to purchase one of the assets and make profits by backrunning (after the transaction is attacked) a transaction to sell the asset.

Although `_slippageFactor` is used in the function `Strategy._safeSwap()`, the amount expected from a swap is calculated through `IPancakeRouter02(_uniRouterAddress).getAmountsOut()`, which depends on the reserves in the swap contract and can be manipulated by flashloans. 

An attacker can therefore perform a sandwich attack by manipulating the reserves in the appropriate router.

Potential Sandwich Attacks

In the contract `Strategy`, the role `FARM_ROLE` has the authority over the following functions:
- `Strategy.deposit()`, which increases shares and may stake or deposit to a farm contract;
- `Strategy.withdraw()`, which decreases shares, may unstake or withdraw from a farm ccntract, and send want tokens to the UPFarm address.

In addition, the role `TIMELOCK_ROLE` has the authority over the following functions:
- `Strategy.earn()`, which collects earn tokens, distributes fees, buybacks tokens, and swaps earned tokens for tokens to be added as liquidity to `uniRouterAddress`;
- `Strategy.setSettings()`, which changes the entrance fee, withdraw fee, controller fee, buyback rate, and slippage factor;
- `Strategy.setUniRouterAddress()`, which decides the address of `uniRouterAddress`;
- `Strategy.setBuyBackAddress()`, which decides the address of `buyBackAddress`;
- `Strategy.setRewardsAddress()`, which decides the address of `rewardsAddress`;
- `Strategy.inCaseTokensGetStuck()`, which sends `ERC20` tokens that are not want tokens or earned tokens to an address;
- `Strategy.wrapBNB()`, which wraps the BNB balance of the contract into tokens;
- `CoreRef.pause()`, which prevents the `deposit()`, `earn()`, and `convertDustToEarned()` functions from being called;
- `CoreRef.unpause()`, which allows the functions disabled from `CoreRef.pause()` to be used again.

The role `GOVERN_ROLE` has the authority over the following function:
- `CoreRef.setCore()`, which updates the core address.

Any compromise to the aforementioned privileged roles may allow the hacker to take advantage of this and obstruct how the contract should operate.

Centralization Related Risks In Strategies

### StakingRewards
In the contract `StakingRewards`, the role `GOVERN_ROLE` has the authority over the following functions:
- `StakingRewards.addPool()`, which adds a liquidity pool;
- `StakingRewards.setPool()`, which updates a pool's allocation points;
- `CoreRef.setCore()`, which updates the core address.

In addition, the role `GUARDIAN_ROLE` has the authority over the following functions:
- `StakingRewards.addPool()`, which adds a liquidity pool;
- `StakingRewards.setPool()`, which updates a pool's allocation points.

The role `TIMELOCK_ROLE` has the authority over the following functions:
- `StakingRewards.updateUpPerBlock()`, which decides how many UP tokens are rewarded per block;
- `StakingRewards.updateEndBlock()`, which decides the last block for rewards;
- `StakingRewards.updateVestingMaster()`, which decides the address for `vestingMaster`;
- `CoreRef.pause()`, which prevents the `deposit()` function from being called;
- `CoreRef.unpause()`, which allows the `deposit()` function to be used again.

### UpFarm
In the contract `UpFarm`, the role `TIMELOCK_ROLE` has the authority over the following functions:
- `UpFarm.add()`, which adds a liquidity pool;
- `UpFarm.set()`, which changes the allocation points of a pool;
- `UpFarm.setVestingMaster()`, which changes the `vestingMaster` address;
- `UpFarm.updateUPPerBlock()`, which decides the number of UP tokens gained per block;
- `UpFarm.inCaseTokensGetStuck()`, which transfers any non-UP tokens in the contract to the owner;

In addition. the role `GOVERN_ROLE` has the authority over the following function:
- `CoreRef.setCore()`, which updates the core address.

### TimelockController
In the contract `TimelockController`, the deployer and contract itself has the role `TIMELOCK_ADMIN_ROLE`, which is the administrative role for the roles `TIMELOCK_ADMIN_ROLE`, `PROPOSER_ROLE`, and `EXECUTOR_ROLE`. This means that they have access to the following functions with respect to the aforementioned roles:
- `AccessControl.grantRole()`, which grants one of the above roles to an address;
- `AccessControl.revokeRole()`, which revokes one of the above roles from an address.

The role `PROPOSER_ROLE` has the authority over the following functions:
- `TimelockController.schedule()`, which schedules an operation;
- `TimelockController.scheduleBatch()`, which schedules a batch of operations;
- `TimelockController.cancel()`, which cancels and operation;
- `AccessControl.renounceRole()`, which gives up their role.

The role `EXECUTOR_ROLE` has the authority over the following functions:
- `TimelockController.execute()`, which executes a scheduled operation;
- `TimelockController.executeBatch()`, which executes a batch of operations;
- `TimelockController.scheduleSet()`, which schedules a change to a liquidity pool's allocation points at an `UpFarm` contract;
- `TimelockController.executeSet()`, which executes a scheduled change to a liquidity pool's allocation points at an `UpFarm` contract;
- `TimelockController.add()`, which adds a liquidity pool to an `UpFarm` contract;
- `TimelockController.earn()`, which causes a `Strategy` contract to collect earned tokens, swap them for liquidity tokens, and then deposit/stake these tokens;
- `TimelockController.farm()`, which causes a `Strategy` contract to deposit/stake tokens;
- `TimelockController.pause()`, which disables the functions `deposit()`, `earn()`, and `convertDustToEarned()` in a `Strategy` contract;
- `TimelockController.unpause()`, which reenables the functions `deposit()`, `earn()`, and `convertDustToEarned()` in a paused `Strategy` contract;
- `TimelockController..wrapBNB()`, which converts a `Strategy` contract's `BNB` balance into `WBNB` tokens.

The contract itself has authority over the following functions, which can be called by having `PROPOSER_ROLE` schedule the operation and `EXECUTOR_ROLE` execute the operation:
- `TimelockController.updateMinDelay()`, which changes the minimum amount of delay for an operation to be executed; 
- `TimelockController.updateMinDelayReduced()`, which changes the minimum amount of delay for `set` to be executed at an `UpFarm` contract.

Any compromise to the aforementioned privileged roles may allow the hacker to take advantage of this and obstruct how the contract should operate.

Centralization Related Risks In Farming and Reward Distributions

### UpToken
In the contract `UpToken`, the the role `GOVERN_ROLE` has the authority over the following functions:
- `UpToken.mint()`, which mints UP tokens to an address.
- `UpToken.increaseCap()`, which adds to the maximum amount of UP tokens there can be;
- `CoreRef.setCore()`, which updates the core address.

### VestingMaster
In the contract `VestingMaster`, the role `GOVERN_ROLE` has authority over the following function:
- `CoreRef.setCore()`, which updates the core address.

In addition, the role `FARM_ROLE` has authority over the following function:
- `VestingMaster.lock()`, which locks up reward amounts for a user.

Any compromise to the aforementioned privileged roles may allow the hacker to take advantage of this and obstruct how the contract should operate.

Centralization Related Risks In Token And Vesting

When users call `deposit()` or `withdraw()`, they can acquire UP tokens as a reward depending on how much they have deposited and for how long. This transfer of rewards is done through the function `safeUPTransfer()`.

However, if there are not enough UP tokens in the contract, users will receive a smaller reward or even no reward. In these situations, the user's `rewardDebt` is still updated, causing them to never be able to acquire these missed rewards.

Users Can Never Claim Missed Rewards

The value of `upPerBlock` can be updated through the function `updateUpPerBlock` by the `_owner` role, which decides the number of UP tokens to be distributed to users. 

However, it does not transfer tokens to the `StakingRewards` contract if `upPerBlock` is increased. It is possible that the contract does not have enough balance to pay users' pending UP tokens.

Non-Guaranteed Token Flow

If `isAutoComp` is set to `true`, such as in `StrategyMars`, then an attacker can use a flashloan attack to steal rewards. 

In the function `deposit()`, a user's shares are calculated before `wantLockedTotal` is changed.
```solidity=104
        uint256 sharesAdded = _wantAmt;
        if (wantLockedTotal > 0 && sharesTotal > 0) {
            sharesAdded = _wantAmt
                .mul(sharesTotal)
                .mul(entranceFeeFactor)
                .div(wantLockedTotal)
                .div(entranceFeeFactorMax);
        }
        sharesTotal = sharesTotal.add(sharesAdded);

        if (isAutoComp) {
            _farm();
        } else {
            wantLockedTotal = wantLockedTotal.add(_wantAmt);
        }
```
When `isAutoComp` is `true`, the `wantLockedTotal` is instead increased by the balance of the contract.
```solidity=127
    function _farm() internal virtual {
        require(isAutoComp, "!isAutoComp");
        uint256 wantAmt = IERC20(wantAddress).balanceOf(address(this));
        wantLockedTotal = wantLockedTotal.add(wantAmt);
```
As such, if `IERC20(wantAddress).balanceOf(address(this))` is higher than `_wantAmt`, then the number of shares created for the user are worth more than they should be.

An attacker can therefore take a flashloan to become a dominant shareholder and withdraw immediately to obtain a large portion of the difference between their deposit and `IERC20(wantAddress).balanceOf(address(this))`.

Flashloan Attack Can Steal Rewards

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user stakes 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived in the contract. However, the user can still withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction.

Incompatibility With Deflationary Tokens

The following functions affects the status of sensitive state variables and should be able to emit events as notifications:
- `deposit()`
- `withdraw()`

Lack of Event Emissions for Significant Transactions

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `deposit()`
- `farm()`
- `earn()`
- `convertDustToEarned()`
- `pause()`
- `unpause()`
- `setSettings()`
- `setGov()`
- `setOnlyGov()`
- `setUniRouterAddress()`
- `setBuyBackAddress()`
- `setRewardsAddress()`
- `inCaseTokensGetStuck()`
- `wrapBNB()`

The functions that are never called internally within the contract should have external visibility. 

Function Visibility Optimization

Users normally earn rewards when `earn()` is called so that earned tokens are swapped into want tokens and staked/deposited. This allows users with shares to claim more than they deposited.

However, when the `isCollect` variable is set to `true`, each time `earn()` is called, the earnings are sent to `rewardsAddress`. Hence when users withdraw their shares, they will never be able to earn these rewards.

Rewards When `isCollect` Is `true`

The `require` statements checks if `isAutoComp` is `true`, but this variable is always set to `true` and never changes.

Redundant Code

The fees set in the constructor of `StrategyMars` can bypass the fee limits defined in `Strategy`.

Limits on Fees

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `earn()`
- `setMarsRouterAddress()`

The functions that are never called internally within the contract should have external visibility. 

The fees set in the constructor of `StrategyPCS` can bypass the fee limits defined in `Strategy`.

In the `addPool()` and `setPool()` functions, the flag '_withUpdate' determines if all the pools will be updated. This reliance might lead to a significant loss of the reward amount.

```solidity
if (_withUpdate) {
    massUpdatePools();
}
```

For an illustration, assume we only have one pool and it has the values `pool.allocPoint == 50` and `totalAllocPoint == 50`. Now we want to add another pool with `pool.allocPoint == 50`.

There will be two scenarios for calculating the pool reward:

Case 1: `withUpdate` is set to `true`.
- Distribute the reward and update the pool.
- Add the given pool information.

Case 2: `withUpdate` is set to `false`:
- Add the given pool information.
 
(Note: While we focused on the `addPool()` function, both the `addPool()` and `setPool()` functions update `totalAllocPoint`, which is used in the calculation of pool rewards in the function `updatePool()`


In Case 1, the reward for the first pool is updated in the call to `updatePool()`
```solidity=166
        upReward = multiplier.mul(upPerBlock).mul(pool.allocPoint).div(
            totalAllocPoint
        );
    }
```
so we have `upReward = multiplier * upPerBlock * 50 / 50 = multiplier * UPPerBlock`.

In Case 2, an update `totalAllocPoint = totalAllocPoint.add(_allocPoint)` is done first. Then when `updatePool()` is called later, the calculation of the reward for the first pool becomes `upReward = multiplier * upPerBlock * 50 / 100`, half of what it is expected to be.

Potential Loss of Pool Rewards

The following areas contain redundant code:

1. (Line 44) The only use of the variable `BONUS_MULTIPLIER` is to multiply an amount in the function `getMultiplier()`. However, the value of `BONUS_MULTIPLIER` is always 1 and never changes, making this multiplication unneeded.
2. (Lines 70-71) The variable `up` is never used except for defining `uptoken`. This variable can be removed and have `uptoken` be defined as `IERC20(_up)`.

In the function `getUPReward()`, the following error message is misspelled:
```solidity=158
        require(
            pool.lastRewardBlock < block.number,
            "StakingReward::getUPReward: Must little than the current block number"
        );
```
It should instead read "Must be lower than the current block number."

Misspelled Error Message

The function `getMultiplier()` is correctly called within the contract.

However, considering that it is a `public` function, it should set `_to` to `endBlock` when `_to > endBlock` and `_from` to `startBlock` when `_from < startBlock` so that external users obtain correct information.

Potential Incorrect Return

The following functions affects the status of sensitive state variables and should be able to emit events as notifications:
- `addPool()`
- `setPool()`

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `addPool()`
- `setPool()`
- `deposit()`
- `withdraw()`
- `emergencyWithdraw()`
- `updateUpPerBlock()`
- `updateEndBlock()`
- `updateVestingMaster()`
- `setPause()`
- `setUnPause()`
  
The functions that are never called internally within the contract should have external visibility. 

After [EIP-1884](https://eips.ethereum.org/EIPS/eip-1884) was included in the Istanbul hard fork, it is not recommended to use `.transfer()` or `.send()` for transferring BNB as these functions have a hard-coded value for gas costs making them obsolete as they are forwarding a fixed amount of gas, specifically `2300`. This can cause issues in case the linked statements are meant to be able to transfer funds to other contracts instead of EOAs.

Usage of `transfer()`

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. 
If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. 

Potential Reentrancy Attack

There are two `SetScheduled` events and the latter one defined is never used.

Unused Event

The following functions affects the status of sensitive state variables and should be able to emit events as notifications:
- `setDevWalletAddress()`
- `executeSet()`
- `add()`
- `earn()`
- `farm()`
- `pause()`
- `unpause()`
- `wrapBNB()`

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `getTimestamp()`
- `getMinDelay()`
- `schedule()`
- `scheduleBatch()`
- `cancel()`
- `execute()`
- `executeBatch()`
- `setDevWalletAddress()`
- `scheduleSet()`
- `executeSet()`
- `withdrawBNB()`
- `withdrawBEP20()`
- `add()`
- `earn()`
- `farm()`
- `pause()`
- `unpause()`
- `wrapBNB()`

The functions that are never called internally within the contract should have external visibility. 

The function `TimelockController.setDevWalletAddress()` checks `tx.origin` before updating `devWalletAddress`:
```solidity=426
        require(tx.origin == devWalletAddress, "tx.origin != devWalletAddress");
```
However, this check can be bypassed if `devWalletAddress` interacts with a malicious contract. For example, the attack vector might look like this:
- `devWalletAddress` -> `MaliciousContract` -> `ExecutorContract` -> `TimelockController.execute()` -> `TimelockController.setDevWalletAddress()`

Improper Usage of `tx.origin` for Authorization 

In the `add()` and `set()` functions, the flag '_withUpdate' determines if all the pools will be updated. This reliance might lead to a significant loss of the reward amount.

```solidity
    if (_withUpdate) {
        massUpdatePools();
    }
```

For an illustration, assume we only have one pool and it has the values `pool.allocPoint == 50` and `totalAllocPoint == 50`. Now we want to add another pool with `pool.allocPoint == 50`.

There will be two scenarios for calculating the pool reward:

Case 1: `withUpdate` is set to `true`.
- Distribute the reward and update the pool.
- Add the given pool information.

Case 2: `withUpdate` is set to `false`:
- Add the given pool information.
 
(Note: While we focused on the `add()` function, both the `add()` and `set()` functions update `totalAllocPoint`, which is used in the calculation of pool rewards in the function `updatePool()`


In Case 1, the reward for the first pool is updated in the call to `updatePool()`
```solidity=163
        uint256 UPReward =
            multiplier.mul(UPPerBlock).mul(pool.allocPoint).div(
                totalAllocPoint
            );
```
so we have `UPReward = multiplier * UPPerBlock * 50 / 50 = multiplier * UPPerBlock`.

In Case 2, an update `totalAllocPoint = totalAllocPoint.add(_allocPoint)` is done first. Then when `updatePool()` is called later, the calculation of the reward for the first pool becomes `UPReward = multiplier * UPPerBlock * 50 / 100`, half of what it is expected to be.

There are instances where `poolInfo[_pid]` is used, but the variable `pool` is already created to point to this structure. 

The function `getMultiplier()` is correctly called within the contract.

However, considering that it is a `public` function, it should set `_from` to `startBlock` when `_from < startBlock` so that external users obtain correct information.

The following functions affects the status of sensitive state variables and should be able to emit events as notifications:
- `add()`
- `set()`

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `poolLength()`
- `add()`
- `set()`
- `pendingUP()`
- `stakedWantTokens()`
- `deposit()`
- `withdrawAll()`
- `emergencyWithdraw()`
- `setVestingMaster()`
- `updateUPPerBlock()`
- `inCaseTokensGetStuck()`

The functions that are never called internally within the contract should have external visibility. 

There is no check to see if a previous pool has the same `want` token and `strat`. If two such pools exist with different `allocPoint` values, it does not make sense for users to deposit into the pool with a lower `allocPoint` value. 

Pools With the Same `want` and `strat`

The following function affects the status of sensitive state variables and should be able to emit events as notifications:
- `increaseCap()`

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `mint()`
- `increaseCap()`

The functions that are never called internally within the contract should have external visibility. 

If the variable `lockedPeriodAmount` is large, not only will the `for` loop in `lock()` be expensive, but a user's `userLockedRewards[user]` record will be very long. 

This can cause expensive transactions or reverts if not enough gas is supplied when the functions `lock()`, `claim()`, and `getVestingAmount()` are called.

Size of `lockedPeriodAmount` Can Cause Expensive Transactions

The following functions are declared as `public` and are not invoked in any of the contracts contained within the project's scope:
- `lock()`
- `claim()`
- `getVestingAmount()`

The functions that are never called internally within the contract should have external visibility. 

UpDeFi

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

39

0

8

0

31