Concur Finance contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L89

Vulnerability details

The MasterChef.add function changes the total pool allocation but does not update other pools first. When other pools are finally updated at some point, then accConcurPerShare will be wrongly computed with their smaller allocPoint / newTotalAllocPoint share even for the period before the new allocation was set.

Users will receive fewer rewards than they should receive after an add call.

Recommended Mitigation Steps

Perform a massUpdatePool in add before changing totalAllocPoint.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L53

Vulnerability details

The Sheler.withdraw function sets the claimed[_token][user] field but does not check if the user is allowed to claim by checking require(!claimed[_token][user], "already claimed").

function withdraw(IERC20 _token, address _to) external override {
    require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
    // @audit doesn't even require claimed[_token][_to] == false
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
    claimed[_token][_to] = true;
    emit ExitShelter(_token, msg.sender, _to, amount);
    _token.safeTransfer(_to, amount);
}

An attacker can withdraw tokens several times.

Recommended Mitigation Steps

Add a require(!claimed[_token][_to] = true, "already claimed") check. (Actually require(!claimed[_token][msg.sender]), see next issue.)

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L164

Vulnerability details

The ConvexStakingWrapper._calcRewardIntegral function makes the d_reward = IERC20(reward.token).balanceOf(address(this)); - reward.remaining amount available for claiming. Then it updates the reward.remaining value to the balance before the distribution.

RewardType memory reward = rewards[_pid][_index];

//get difference in balance and remaining rewards
//getReward is unguarded so we use remaining to keep track of how much was actually claimed
uint256 bal = IERC20(reward.token).balanceOf(address(this));
uint256 d_reward = bal - reward.remaining;

// ...
IERC20(reward.token).transfer(address(claimContract), d_reward);

// ...

//update remaining reward here since balance could have changed if claiming
if (bal != reward.remaining) {
    reward.remaining = uint128(bal);
}

rewards[_pid][_index] = reward;

It's unclear what the reasoning is but it leads to delayed distributions and breaks withdrawals as uint256 d_reward = bal - reward.remaining; will underflow next time.

POC

• The first time _calcRewardIntegral is called, bal = IERC20(reward.token).balanceOf(address(this)) := INITIAL_BALANCE, d_reward = bal - 0 = INITIAL_BALANCE.
• The entire balance d_reward is transferred out of the contract
• reward.remaining = INITIAL_BALANCE is set.
• User tries to withdraw tokens. It calls _checkpoint(_pid, msg.sender) which calls _calcRewardIntegral() which computes d_reward = IERC20(reward.token).balanceOf(address(this)) - INITIAL_BALANCE which will most likely underflow now as the current balance is 0 and thus < reward.remaining = INITIAL_BALANCE.
• the transaction reverts, user cannot withdraw.

Recommended Mitigation Steps

Clarify how the reward.remaining variable is supposed to work and fix it.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L55

Vulnerability details

The Sheler.withdraw function sets the claimed[_token][user] field but uses the shares of msg.sender. An attacker can withdraw tokens several times passing different _to addresses, each time the msg.sender's shares will be used to receive tokens.

function withdraw(IERC20 _token, address _to) external override {
    require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
    // @audit uses `msg.sender`'s share but sets `claimed` for _to! can claim for many `_to`s
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
    claimed[_token][_to] = true;
    emit ExitShelter(_token, msg.sender, _to, amount);
    _token.safeTransfer(_to, amount);
}

Recommended Mitigation Steps

Change withdraw to:

function withdraw(IERC20 _token, address _to) external override {
    require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
+   require(!claimed[_token][msg.sender], "already claimed");
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
-   claimed[_token][_to] = true;
+   claimed[_token][msg.sender] = true;
    emit ExitShelter(_token, msg.sender, _to, amount);
    _token.safeTransfer(_to, amount);
}

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L164

Vulnerability details

The ConvexStakingWrapper contract uses several reward pool tokens rewards[_pid][_index].token and it can be that the same token is used for different _pids. Indeed, the CVX/CRV tokens are always at index 0 and 1.

The rewards will be distributed to the first pool id (_pid) calling checkpoint() -> _calcRewardIntegral(). The other pools don't receive any rewards.

function _calcRewardIntegral(
    uint256 _pid,
    uint256 _index,
    address _account,
    uint256 _balance,
    uint256 _supply
) internal {
    RewardType memory reward = rewards[_pid][_index];

    // @audit first pid to call this receives the current reward token balance
    uint256 bal = IERC20(reward.token).balanceOf(address(this));
    // ...

    uint256 d_reward = bal - reward.remaining;
    IERC20(reward.token).transfer(address(claimContract), d_reward);

    // ...
}

Recommended Mitigation Steps

If several pools use the same reward token, it should be distributed fairly among these pools, similar to the allocation points in a MasterChef contract.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L171

Vulnerability details

Impact

The StakingRewards.rewardToken can be recovered by calling recoverERC20(stakingRewards.rewardToken, stakingRewards.rewardToken.balanceOf(stakingRewards)). It only checks that tokenAddress != stakingToken.

function recoverERC20(address tokenAddress, uint256 tokenAmount)
    external
    onlyOwner
{
    require(
        tokenAddress != address(stakingToken),
        "Cannot withdraw the staking token"
    );
    IERC20(tokenAddress).safeTransfer(owner(), tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

Note that this contract seems to keep the rewards in the contract as the notifyRewardAmount function has code restricting the rewardRate by balance = rewardsToken.balanceOf(address(this));.

The owners can steal the users' rewards.

Recommended Mitigation Steps

function recoverERC20(address tokenAddress, uint256 tokenAmount)
    external
    onlyOwner
{
    require(
+       tokenAddress != address(rewardToken) &&
        tokenAddress != address(stakingToken),
-       "Cannot withdraw the staking token"
+       "Cannot withdraw the staking and reward tokens"
    );
    IERC20(tokenAddress).safeTransfer(owner(), tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L34

Vulnerability details

The Sheler.donate function transferFroms _amount and adds the entire _amount to savedTokens[_token]. But the actual received token amount from the transfer can be less for fee-on-transfer tokens.

The last person to withdraw will not be able to as withdraw uses a share computation for the entire savedTokens[_token] amount. The calculated amount will then be higher than the actual contract balance.

function donate(IERC20 _token, uint256 _amount) external {
    require(activated[_token] != 0, "!activated");
    savedTokens[_token] += _amount;
    // @audit fee-on-transfer. then fails for last person in `withdraw`
    _token.safeTransferFrom(msg.sender, address(this), _amount);
}

function withdraw(IERC20 _token, address _to) external override {
    // @audit percentage on storage var, not on actual balance
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
    // @audit amount might not be in contract anymore as savedTokens[_token] is over-reported
    _token.safeTransfer(_to, amount);
}

Recommended Mitigation Steps

In donate, add only the actual transferred amounts (computed by post-transfer balance - pre-transfer balance) to savedTokens[_token].

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L157

Vulnerability details

The MasterChef.deposit function does not transferFrom the _amount token amount from the caller. It's unclear if there are other callers besides the ConvexStakingWrapper as there could be multiple depositor judging from the isDepositor mapping instead of a single address depositor variable.

Recommended Mitigation Steps

Make sure the contract is not used on its own and all depositors require some token commitment from the user.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L161

Vulnerability details

Impact

The StakingRewards.notifyRewardAmount function receives a reward amount and extends the current reward end time to now + rewardsDuration. It rebases the currently remaining rewards + the new rewards (reward + leftover) over this new rewardsDuration period.

function withdraw(IERC20 _token, address _to) external override {
    require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
    // @audit uses `msg.sender`'s share but sets `claimed` for _to! can claim for many `_to`s
    uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
    claimed[_token][_to] = true;
    emit ExitShelter(_token, msg.sender, _to, amount);
    _token.safeTransfer(_to, amount);
}

This can lead to a dilution of the reward rate and rewards being dragged out forever by malicious new reward deposits.

POC

Imagine the current rewardRate is 1000 rewards / rewardsDuration. 20% of the rewardsDuration passed, i.e., now = lastUpdateTime + 20% * rewardsDuration. A malicious actor notifies the contract with a reward of 0: notifyRewardAmount(0). Then the new rewardRate = (reward + leftover) / rewardsDuration = (0 + 800) / rewardsDuration = 800 / rewardsDuration. The rewardRate just dropped by 20%. This can be repeated infinitely. After another 20% of reward time passed, they trigger notifyRewardAmount(0) to reduce it by another 20% again: rewardRate = (0 + 640) / rewardsDuration = 640 / rewardsDuration.

Recommended Mitigation Steps

Imo, the rewardRate should never decrease by a notifyRewardAmount call. Consider not extending the reward payouts by rewardsDuration on every call. periodFinish probably shouldn't change at all, the rewardRate should just increase by rewardRate += reward / (periodFinish - block.timestamp).

Alternatively, consider keeping the rewardRate constant but extend periodFinish time by += reward / rewardRate.