QA Report

[code423n4]

Codebase Impressions & Summary

Overall, code quality was fair. A number of contracts were taken from various sources, such as StakingRewards, Masterchef and the ConvexStakingWrapper. Modifications were made to include custom features like taking a 20% fee on CVX and CRV rewards for the treasury, and to not require stake token transfers for deposits / withdrawals into the Masterchef contract.

I found 10 high severity issues, majority of which are found in the Masterchef contract. They were simple logic bugs that would have been discovered with unit tests.

In addition, I made 2 medium severity, 7 low severity and 1 non-critical findings.

Note that during the contest, an example shelter client was added and pushed to a new branch for wardens to understand how the shelter would operate. The integration of the ConvexStakingWrapper with the Shelter in that branch has a few bugs, but I assume it is outside the current contest scope to report them.

Due to the number of issues raised, I strongly recommend the team to write unit tests for their contracts, and to consider running a mitigation contest.

Low Severity Findings

L01: Masterchef: pendingConcur() shows increasing reward amounts after mining period ends

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L113-L124

Description

Even though rewards distribution cease after endBlock, pendingConcur() will calculate as if reward distribution has not.

Distribution of rewards will cease after endBlock, but pendingConcur() will show increasing pending rewards because it does not account for endBlock.

Recommended Mitigation Steps

function pendingConcur(uint _pid, address _user) external view returns (uint) {
    ...
    // take the minimum of endBlock and currentBlock
    uint endRewardBlock = endBlock >= block.number ? block.number : endBlock;
    if (endRewardBlock > pool.lastRewardBlock && lpSupply != 0) {
        uint multiplier = getMultiplier(pool.lastRewardBlock, endRewardBlock);
        ...
    }
    ...
}

L02: Masterchef: Incorrect comment on endBlock

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L52-L53

Description

uint public endBlock; // The block number when mining starts. is incorrect, as it should be the end of the mining period, not the start. Its comment applies to startBlock.

Note that uint public startBlock does not have a comment. Consider adding it.

Recommended Mitigation Steps

uint public startBlock; // The block number when mining starts.
uint public endBlock; // The block number when mining ends.

L03: Masterchef: safeConcurTransfer() potentially reverts for zero amount

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L205-L210

Description

If the contract has zero concur tokens, the following may revert because of zero amount. This is of course dependent on the concur token implementation.

// couple of lines omitted
transferSuccess = concur.transfer(_to, concurBalance);
require(transferSuccess, "safeConcurTransfer: transfer failed"); 

L04: Masterchef: Ensure 0 _allocationPoints if pool added after endBlock in add()

Description

While a pool can (and should be allowed) to be added after endBlock, no rewards can be distributed after endBlock. Hence, it would be good to ensure _allocationPoints is zero when the pool is added, because a non-zero value comes with the expectation that the pool will be receiving rewards (worse still, have concur tokens transferred to the contract, which will be permanently locked).

Recommended Mitigation Steps

// in add()
if (block.number > endBlock) require(_allocationPoints == 0, “mining period ended”);

L05: ConvexStakingWrapper: Small rounding error in _calcRewardIntegral()

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L179-L180

Description

The treasury takes a 20% fee of rewards. The calculation will possibly leave 1 wei unaccounted for.

IERC20(reward.token).transfer(treasury, d_reward / 5);
d_reward = (d_reward * 4) / 5;

For instance, assume d_reward = 21. The treasury receives 4 wei while the user receives 16 wei, leaving 1 wei unaccounted for.

Recommended Mitigation Steps

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

L06: USDMPegRecovery: 40M or 4M threshold?

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L100

Description

The README says “Once 40m USDM is deposited, 3Crv side of the contract starts accepting deposits.” However, the code accepts 3CRV deposits after 4M USDM is deposited instead.

Recommended Mitigation Steps

Specify the threshold as an internal constant, and use underscores for readability. I also recommend double-checking the values of declared variables in all contracts, such as step and concurPerBlock.

uint256 internal constant MIN_USDM_AMOUNT = 40_000_000e18;
require(totalLiquidity.usdm > MIN_USDM_AMOUNT, "usdm low");

// or
require(totalLiquidity.usdm > 40_000_000e18, "usdm low");

L07: StakingRewards: Incorrect revert statement in setRewardsDistribution()

Line References

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol#L191-L194

Description

setRewardsDistribution() has the following check:

require(
  block.timestamp > periodFinish,
  "Previous rewards period must be complete before changing the duration for the new period"
);

The statement is incorrect because it’s rewardsDistribution that is being changed, not the rewards duration.

Recommended Mitigation Steps

Actually, the check is redundant, because there is no harm changing rewardsDistribution while distribution is ongoing. I suggest removing the check entirely. Otherwise, change the comment to

"Previous rewards period must be complete before changing rewardsDistribution"

Non-Critical Findings

NC01: Masterchef: RADSs → Concurs

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L25

Rename RADSs to Concurs

[GalloDaSballo]

L01: Masterchef: pendingConcur() shows increasing reward amounts after mining period ends Valid finding

L02: Masterchef: Incorrect comment on endBlock Non-critical IMO

L03: Masterchef: safeConcurTransfer() potentially reverts for zero amount I don't believe it will cause issues, but think 0 check is low per industry standard

L04: Masterchef: Ensure 0 _allocationPoints if pool added after endBlock in add() Disagree as adding after end will give them no point anyway and the warden didn't prove a way to sidestep that

L05: ConvexStakingWrapper: Small rounding error in _calcRewardIntegral() After further consideration I agree

L06: USDMPegRecovery: 40M or 4M threshold? I feel like this is the only case where I'd give low vs non-critical as the comment and the code have a meaningful, and significant difference for the end users

L07: StakingRewards: Incorrect revert statement in setRewardsDistribution() Disagree with severity

NC01: Masterchef: RADSs → Concurs Valid finding

Report has plenty of content, formatting is good, I think most findings are over-emphasized though and under further scrutiny this is basically equivalent to 4 findings

[GalloDaSballo]

Adding #137 does make the report more well rounded and adding #136 makes this the most interesting report thus far, 6.5 findings at this time

[GalloDaSballo]

6++ with very good formatting

[GalloDaSballo]

L01: Masterchef: pendingConcur() shows increasing reward amounts after mining period ends Low

L02: Masterchef: Incorrect comment on endBlock Non-Critical

L03: Masterchef: safeConcurTransfer() potentially reverts for zero amount Low

L05: ConvexStakingWrapper: Small rounding error in _calcRewardIntegral() Low

L06: USDMPegRecovery: 40M or 4M threshold? Low

L07: StakingRewards: Incorrect revert statement in setRewardsDistribution() Non Critical

NC01: Masterchef: RADSs → Concurs Non-Critical

137 -> Non-critical

136 -> Low Severity

[liveactionllama]

Just a note that C4 is excluding any invalid entries from the official report.