Fei Tribechief

Description

When withdrawing a single deposit, the reward debt is updated:

contracts/staking/TribalChief.sol:L468-L474

uint128 virtualAmountDelta = uint128( ( amount * poolDeposit.multiplier ) / SCALE_FACTOR );

// Effects
poolDeposit.amount -= amount;
user.rewardDebt = user.rewardDebt - toSigned128(user.virtualAmount * pool.accTribePerShare) / toSigned128(ACC_TRIBE_PRECISION);
user.virtualAmount -= virtualAmountDelta;
pool.virtualTotalSupply -= virtualAmountDelta;

Instead of the user.virtualAmount in reward debt calculation, the virtualAmountDelta should be used. Because of that bug, the reward debt is much lower than it would be, which means that the reward itself will be much larger during the harvest. By making multiple deposit-withdraw actions, any user can steal all the Tribe tokens from the contract.

Recommendation

Use the virtualAmountDelta instead of the user.virtualAmount.

Description

TribalChief.updatePool will revert in the case totalAllocPoint = 0, which will essentially cause users’ funds and rewards to be locked.

Recommendation

TribalChief.add and TribalChief.set should assert that totalAllocPoint > 0. A similar validation check should be added to TribalChief.updatePool as well.

Description

When a user deposits funds to a pool, the current multiplier in use for this pool is being stored locally for this deposit. The value that is used later in a withdrawal operation is the local one, and not the one that is changing when a governor calls governorAddPoolMultiplier. It means that a decrease in the multiplier value for a given pool does not affect users that already deposited, but an increase does. Users that had already deposited should have the right to withdraw their funds when the multiplier for their pool increases by the governor.

Examples

code/contracts/staking/TribalChief.sol:L143-L158

function governorAddPoolMultiplier(
    uint256 _pid,
    uint64 lockLength,
    uint64 newRewardsMultiplier
) external onlyGovernor {
    PoolInfo storage pool = poolInfo[_pid];
    uint256 currentMultiplier = rewardMultipliers[_pid][lockLength];
    // if the new multplier is less than the current multiplier,
    // then, you need to unlock the pool to allow users to withdraw
    if (newRewardsMultiplier < currentMultiplier) {
        pool.unlocked = true;
    }
    rewardMultipliers[_pid][lockLength] = newRewardsMultiplier;

    emit LogPoolMultiplier(_pid, lockLength, newRewardsMultiplier);
}

Recommendation

Replace the < operator with > in TribalChief line 152.

Description

TribalChief consists of multiple unsafe down-casting operations. While the usage of types that can be packed into a single storage slot is more gas efficient, it may introduce hidden risks in some cases that can lead to loss of funds.

Examples

Various instances in TribalChief, including (but not necessarily only) :

code/contracts/staking/TribalChief.sol:L429

user.rewardDebt = int128(user.virtualAmount * pool.accTribePerShare) / toSigned128(ACC_TRIBE_PRECISION);

code/contracts/staking/TribalChief.sol:L326

pool.accTribePerShare = uint128(pool.accTribePerShare + ((tribeReward * ACC_TRIBE_PRECISION) / virtualSupply));

code/contracts/staking/TribalChief.sol:L358

userPoolData.rewardDebt += int128(virtualAmountDelta * pool.accTribePerShare) / toSigned128(ACC_TRIBE_PRECISION);

Recommendation

Given the time constraints of this audit engagement, we could not verify the implications and provide mitigation actions for each of the unsafe down-castings operations. However, we do recommend to either use numeric types that use 256 bits, or to add proper validation checks and handle these scenarios to avoid silent over/under-flow errors. Keep in mind that reverting these scenarios can sometimes lead to a denial of service, which might be harmful in some cases.

Description

EthCompoundPCVDeposit accepts ETH via receive(). Anyone can call EthCompoundPCVDeposit.deposit() to mint CToken for the contracts ETH balance.

The CToken to be used is configured on EthCompoundPCVDeposit deployment. It is not checked, whether the provided CToken address is actually a valid CToken.

If the configured CToken ceases to work correctly (e.g. CToken.mint|redeem* disabled or the configured CToken address is invalid), ETH held by the contract may be locked up.

Recommendation

Similar to EthLidoPCVDeposit add a method witdrawETH, access-restricted to onlyPCVController, that allows recovering ETH from the EthCompoundPCVDeposit contract in case the CToken contract throws. (Consider moving this functionality to PCVDeposit where withdrawERC20 is implemented to avoid having to implement this over and over again)

In CompoundPCVDepositBase consider verifying, that the CToken constructor argument is actually a valid CToken by checking require(ctoken.isCToken(), "not a valid CToken").

Description

When the TribalChief governor decreases the ratio between the allocation point (PoolInfo.allocPoint) and the total allocation point (totalAllocPoint) for a specific pool (either be directly decreasing PoolInfo.allocPoint of a given pool, or by increasing this value for other pools), the total reward for this pool is decreased as well. Depositors should be able to withdraw their funds immediately after this kind of change.

Examples

code/contracts/staking/TribalChief.sol:L252-L261

function set(uint256 _pid, uint128 _allocPoint, IRewarder _rewarder, bool overwrite) public onlyGovernor {
    totalAllocPoint = (totalAllocPoint - poolInfo[_pid].allocPoint) + _allocPoint;
    poolInfo[_pid].allocPoint = _allocPoint.toUint64();

    if (overwrite) {
        rewarder[_pid] = _rewarder;
    }

    emit LogSetPool(_pid, _allocPoint, overwrite ? _rewarder : rewarder[_pid], overwrite);
}

Recommendation

Make sure that depositors’ funds are unlocked for pools that affected negatively by calling TribalChief.set.

Description

When the governor updates the block reward tribalChiefTribePerBlock the new reward is applied for the outstanding duration of blocks in updatePool. This means, if a pool hasn’t updated in a while (unlikely) the new block reward is retrospectively applied to the pending duration instead of starting from when the block reward changed.

Examples

• rewards calculation

code/contracts/staking/TribalChief.sol:L323-L327

if (virtualSupply > 0) {
    uint256 blocks = block.number - pool.lastRewardBlock;
    uint256 tribeReward = (blocks * tribePerBlock() * pool.allocPoint) / totalAllocPoint;
    pool.accTribePerShare = uint128(pool.accTribePerShare + ((tribeReward * ACC_TRIBE_PRECISION) / virtualSupply));
}

• updating the block reward

code/contracts/staking/TribalChief.sol:L111-L116

/// @notice Allows governor to change the amount of tribe per block
/// @param newBlockReward The new amount of tribe per block to distribute
function updateBlockReward(uint256 newBlockReward) external onlyGovernor {
    tribalChiefTribePerBlock = newBlockReward;
    emit NewTribePerBlock(newBlockReward);
}

Recommendation

It is recommended to update pools before changing the block reward. Document and make users aware that the new reward is applied to the outstanding duration when calling updatePool.

Description

Duplicate import for SafeERC20.

Examples

code/contracts/staking/TribalChief.sol:L7-L8

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

Recommendation

Remove duplicate import line.

Description

The method resetRewards silently resets a pools tribe allocation.

Examples

code/contracts/staking/TribalChief.sol:L263-L275

/// @notice Reset the given pool's TRIBE allocation to 0 and unlock the pool. Can only be called by the governor or guardian.
/// @param _pid The index of the pool. See `poolInfo`.   
function resetRewards(uint256 _pid) public onlyGuardianOrGovernor {
    // set the pool's allocation points to zero
    totalAllocPoint = (totalAllocPoint - poolInfo[_pid].allocPoint);
    poolInfo[_pid].allocPoint = 0;
   
    // unlock all staked tokens in the pool
    poolInfo[_pid].unlocked = true;

    // erase any IRewarder mapping
    rewarder[_pid] = IRewarder(address(0));
}

Recommendation

For transparency and to create an easily accessible audit trail of events consider emitting an event when resetting a pools allocation.

Recommendation

Stick to the original upstream interface names to make clear with which external system the contract interacts with. Rename CEth to CEther. See original upstream interface name.

code/contracts/pcv/compound/EthCompoundPCVDeposit.sol:L6-L8

interface CEth {
    function mint() external payable;
}

Recommendation

The ctoken address provided when deploying a new *CompoundPCVDeposit is never validated. Consider adding the following check: require(_cToken.isCToken, "not a valid CToken").

code/contracts/pcv/compound/CompoundPCVDepositBase.sol:L25-L30

constructor(
    address _core,
    address _cToken
) CoreRef(_core) {
    cToken = CToken(_cToken);
}

Recommendation

Currently, the PCV flavor is only unit-tested using a mocked CToken. Consider providing integration tests that actually integrate and operate it in a compound test environment.

Provide a specification. & documentation describing the roles and functionality of the contract. Who deployes the PCVDeposit contract? Who Deploys the CToken and therefore may be in control of certain adminOnly functions of the CToken? What are the requirements for a CToken to be usable with CompoundPCVDeposit (listed/unlisted, …)? Who has the potential power to borrow assets on behalf of the collateral provided?

Recommendation

Constant state variables that are not initialized with the constructor can be constant instead of immutable.

code/contracts/staking/TribalChief.sol:L88-L90

uint256 private immutable ACC_TRIBE_PRECISION = 1e12;
/// exponent for rewards multiplier
uint256 public immutable SCALE_FACTOR = 1e18;

Description

Users should be notified if the pool gets unlocked during a call to governorAddPoolMultiplier. Consider emitting a PoolLocked(false) event.

code/contracts/staking/TribalChief.sol:L143-L158

function governorAddPoolMultiplier(
    uint256 _pid,
    uint64 lockLength,
    uint64 newRewardsMultiplier
) external onlyGovernor {
    PoolInfo storage pool = poolInfo[_pid];
    uint256 currentMultiplier = rewardMultipliers[_pid][lockLength];
    // if the new multplier is less than the current multiplier,
    // then, you need to unlock the pool to allow users to withdraw
    if (newRewardsMultiplier < currentMultiplier) {
        pool.unlocked = true;
    }
    rewardMultipliers[_pid][lockLength] = newRewardsMultiplier;

    emit LogPoolMultiplier(_pid, lockLength, newRewardsMultiplier);
}

Description

When TribalChief.withdrawAllAndHarvest is executed, there’s a redundant invocation of TribalChief.updatePool that caused by TribalChief._harvest, that can be moved to TribalChief.harvest instead.

Examples

code/contracts/staking/TribalChief.sol:L485-L515

function _harvest(uint256 pid, address to) private {
    updatePool(pid);
    PoolInfo storage pool = poolInfo[pid];
    UserInfo storage user = userInfo[pid][msg.sender];

    // assumption here is that we will never go over 2^128 -1
    int256 accumulatedTribe = int256( uint256(user.virtualAmount) * uint256(pool.accTribePerShare) ) / int256(ACC_TRIBE_PRECISION);

    // this should never happen
    require(accumulatedTribe >= 0 || (accumulatedTribe - user.rewardDebt) < 0, "negative accumulated tribe");

    uint256 pendingTribe = uint256(accumulatedTribe - user.rewardDebt);

    // if pending tribe is ever negative, revert as this can cause an underflow when we turn this number to a uint
    require(pendingTribe.toInt256() >= 0, "pendingTribe is less than 0");

    // Effects
    user.rewardDebt = int128(accumulatedTribe);

    // Interactions
    if (pendingTribe != 0) {
        TRIBE.safeTransfer(to, pendingTribe);
    }

    IRewarder _rewarder = rewarder[pid];
    if (address(_rewarder) != address(0)) {
        _rewarder.onSushiReward( pid, msg.sender, to, pendingTribe, user.virtualAmount);
    }

    emit Harvest(msg.sender, pid, pendingTribe);
}