Concur Finance contest - kirk-baird's results

Handle

kirk-baird

Vulnerability details

Impact

Incorrect accounting in _calcRewardIntegral() causes the rewards to be locked and will create an overflow that reverts for future calls.

The impact is a significant loss in each rewards token in terms of value and _calcRewardIntegral() cannot be called again until we have transferred sufficient balance of each rewards token to ConvexStakingWrapper to account for the rewards already claimed.

Proof of Concept

_calcRewardIntegral()

    function _calcRewardIntegral(
        uint256 _pid,
        uint256 _index,
        address _account,
        uint256 _balance,
        uint256 _supply
    ) internal {
        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;
        // send 20 % of cvx / crv reward to treasury
        if (reward.token == cvx || reward.token == crv) {
            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = (d_reward * 4) / 5;
        }
        IERC20(reward.token).transfer(address(claimContract), d_reward);

        if (_supply > 0 && d_reward > 0) {
            reward.integral =
                reward.integral +
                uint128((d_reward * 1e20) / _supply);
        }

        //update user integrals
        uint256 userI = userReward[_pid][_index][_account].integral;
        if (userI < reward.integral) {
            userReward[_pid][_index][_account].integral = reward.integral;
            claimContract.pushReward(
                _account,
                reward.token,
                (_balance * (reward.integral - userI)) / 1e20
            );
        }

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

        rewards[_pid][_index] = reward;
    }

The core of this bug stems from setting the remaining rewards to the initial contract balance i.e.reward.remaining = uint128(bal); even after we have transferred the rewards to the claimContract.

Example

Say we have reward.token = tokenA, tokenA.balanceOf(ConvexStakingWrapper) = 100 and rewards[pid][tokena]remaining = 0 then we do an event that calls _calcRewardIntegral():

• bal = 100
• d_reward = 100 - 0 = 100
• IERC20(reward.token).transfer(address(claimContract), d_reward); occurs making the tokenA.balanceOf(ConvexStakingWrapper) = 0
• reward.remaining = bal = 100

Now say the contract accrues 50 of tokenA as rewards so tokenA.balanceOf(ConvexStakingWrapper) = 50 and an event happens so do _calcRewardIntegral():

• bal = 50
• uint256 d_reward = bal - reward.remaining; is 50 - 100 = -50 this is an overflow and reverts

Thus, we are unable to make future calls to this contract with this pid until the balance of tokenA is greater than 100.

Recommended Mitigation Steps

Consider, removing reward.remaining entirely. This will prevent future calls from overflowing when there are not sufficient rewards. However, this will no longer track the individual reward amounts for each (pid, index) pair.

Handle

kirk-baird

Vulnerability details

Impact

The reward calculations do not sufficiently account for different Curve LPs having the same rewards token.

As a result a you may take the rewards of one pid by using another with the same rewards tokens addresses. Since all tokens have CVX and CVR tokens as rewards these rewards are essentially pool and can be claimed by pid.

Proof of Concept

The function _calcRewardIntegral() simply uses the balance of the rewards token to determine how many rewards have been earnt by a pool. e.g.

uint256 bal = IERC20(reward.token).balanceOf(address(this));

Since reward.token may be CRV or CVX or any other common token e.g. USDC these rewards will be attributed to the pid that has been passed by the user when calling withdraw() ordeposit().

To fully exploit this vulnerability a user may do the following:

• addRewards(pid) where pid is one that has not previously been added
• deposit(pid, 10) thus the user will have 100% of the total supply of LP for this pid
• wait for some rewards to accrue on some other pid pools which will transfer CVX and CRV to this contract
• withdraw(10) this will call _calcRewardIntegral() and will transfer 4/5ths of the balance of the contract to the claim contract and attribute all of this balance to the attacker who may then withdraw the rewards.

Recommended Mitigation Steps

The token rewards need to be differentiated between which pool they came from. One option is to have a different contract for each pid however this was the situation we were trying to improve upon.

Lines of code

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

Vulnerability details

Impact

Incorrect accounting of rewards in updatePool() when dealing with endBlock will result in insufficient rewards being transferred to the users.

Proof of Concept

Rewards are generated per block between startBlock and endBlock. However if block.number >= endBlock then no rewards will be accrued.

This does not account for the rewards between pool.lastRewardBlock and endBlock if pool.lastRewardBlock < endBlock.

For example consider the case where pool.lastBlockReward = 10 and endBlock = 20. If we call updatePool() with block.number = 21 we will set pool.lastReward lock = 21 and not accrue any rewards.

Recommended Mitigation Steps

Consider adding extra conditional statements to accrue rewards for the conditions:

• block.number >= endBlock
• and pool.lastRewardBlock < endBlock

Here we would need to accrue endBlock - pool.lastRewardBlock worth of block rewards.

Handle

kirk-baird

Vulnerability details

Impact

The function claimRewards() is vulnerable to reentrancy. An attacker is able to drain the entire balance of any token for which they have rewards.

Note this attack requires the user to be able to gain control of execution as the to address in the ERC20 transfer() call, which is possible for many ERC20 implementations such as those that include any of the extensions ERC777, ERC1155 or ERC223.

Proof of Concept

claimRewards()

    function claimRewards(address[] calldata _tokens) external override {
        for (uint256 i = 0; i < _tokens.length; i++) {
            uint256 getting = reward[msg.sender][_tokens[i]];
            IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
            reward[msg.sender][_tokens[i]] = 0;
        }
    }

The function updates the user rewards after making an external call, which may allow an attacker to reenter the contract and still have their original balance.

Steps:

1. Use the protocol and earn some rewards (i.e. pushReward())
2. claimRewards([token]) and gain control of the execution during IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
3. repeat step 2) until the balance of the contract is less than reward[msg.sender][_tokens[i]];

Recommended Mitigation Steps

We recommend using the check-effects-interactions pattern. This involved doing all state changes and checks before making external calls.

Consider updating the function to the following.

    function claimRewards(address[] calldata _tokens) external override {
        for (uint256 i = 0; i < _tokens.length; i++) {
            uint256 getting = reward[msg.sender][_tokens[i]];
            reward[msg.sender][_tokens[i]] = 0;
            IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
        }
    }

Handle

kirk-baird

Vulnerability details

Impact

The function _calcRewardIntegral() does not account for rounding issues when determining the balance to transfer to the treasury.

Proof of Concept

_calcRewardIntegral()

tegral(
        uint256 _pid,
        uint256 _index,
        address _account,
        uint256 _balance,
        uint256 _supply
    ) internal {
        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;
        // send 20 % of cvx / crv reward to treasury
        if (reward.token == cvx || reward.token == crv) {
            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = (d_reward * 4) / 5;
        }
        IERC20(reward.token).transfer(address(claimContract), d_reward);
...

Here the calculations IERC20(reward.token).transfer(treasury, d_reward / 5); and d_reward = (d_reward * 4) / 5; will result in a rounding error if d_reward is not a multiple of 5.

Consider the example where d_reward = 1 d_reward / 5 = 0 (d_reward * 4) / 5 = 0 Thus, we will leak some token value.

Recommended Mitigation Steps

Consider the updating the two lines of code to account for the rounding issues. The following code accounts for the rounding issues.

            uint256 treasuryRewards = d_reward / 5;
            IERC20(reward.token).transfer(treasury, treasuryRewards);
            d_reward = d_reward - treasuryRewards;