Concur Finance contest - hickuphh3's results

Lines of code

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

Vulnerability details

Impact

If a pool’s deposit fee is non-zero, it is subtracted from the amount to be credited to the user.

if (pool.depositFeeBP > 0) {
  uint depositFee = _amount.mul(pool.depositFeeBP).div(_perMille);
  user.amount = SafeCast.toUint128(user.amount + _amount - depositFee);
}

However, the deposit fee is not credited to anyone, leading to permanent lockups of deposit fees in the relevant depositor contracts (StakingRewards and ConvexStakingWrapper for now).

Proof of Concept

Example 1: ConvexStakingWrapper

Assume the following

• The curve cDai / cUSDC / cUSDT LP token corresponds to pid = 1 in the convex booster contract.
• Pool is added in Masterchef with depositFeeBP = 100 (10%).

1. Alice deposits 1000 LP tokens via the ConvexStakingWrapper contract. A deposit fee of 100 LP tokens is charged. Note that the deposits mapping of the ConvexStakingWrapper contract credits 1000 LP tokens to her.
2. However, Alice will only be able to withdraw 900 LP tokens. The 100 LP tokens is not credited to any party, and is therefore locked up permanently (essentially becomes protocol-owned liquidity). While she is able to do requestWithdraw() for 1000 LP tokens, attempts to execute withdraw() with amount = 1000 will revert because she is only credited 900 LP tokens in the Masterchef contract.

Example 2: StakingRewards

• CRV pool is added in Masterchef with depositFeeBP = 100 (10%).

1. Alice deposits 1000 CRV into the StakingRewards contract. A deposit fee of 100 CRV is charged.
2. Alice is only able to withdraw 900 CRV tokens, while the 100 CRV is not credited to any party, and is therefore locked up permanently.

These examples are non-exhaustive as more depositors can be added / removed from the Masterchef contract.

Recommended Mitigation Steps

I recommend shifting the deposit fee logic out of the masterchef contract into the depositor contracts themselves, as additional logic would have to be added in the masterchef to update the fee recipient’s state (rewardDebt, send pending concur rewards, update amount), which further complicates matters. As the fee recipient is likely to be the treasury, it is also not desirable for it to accrue concur rewards.

if (pool.depositFeeBP > 0) {
  uint depositFee = _amount.mul(pool.depositFeeBP).div(_perMille);
  user.amount = SafeCast.toUint128(user.amount + _amount - depositFee);
  UserInfo storage feeRecipient = userInfo[_pid][feeRecipient];
  // TODO: update and send feeRecipient pending concur rewards
  feeRecipient.amount = SafeCast.toUint128(feeRecipient.amount + depositFee);
  // TODO: update fee recipient's rewardDebt
}

Lines of code

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

Vulnerability details

Impact

withdraw() assigns sets claimed to true for the desired destination address. This means a user can make multiple claims by withdrawing to multiple addresses.

Recommended Mitigation Steps

Change _to to msg.sender.

claimed[_token][msg.sender] = true;

Lines of code

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

Vulnerability details

Impact

The intended party for crediting the deposit to should be the _recipient, but it is selected as _msgSender(), which would be the depositor contracts (ConvexStakingWrapper and StakingRewards).

UserInfo storage user = userInfo[_pid][_msgSender()];

This means that everyone’s amounts are grouped together, breaking reward distribution calculations.

Recommended Mitigation Steps

Switch _msgSender() to _receipient.

UserInfo storage user = userInfo[_pid][_receipient];

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L175-L182 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L201-L204

Vulnerability details

Impact

bal = IERC20(reward.token).balanceOf(address(this)); is not updated after making token transfers to the claimContract (and treasury if reward token is CRV / CVX). Hence, reward.remaining will be incorrectly updated to bal, causing subsequent _calcRewardIntegral() calls to fail while the reward token balance does not exceed the old balance.

if (bal != reward.remaining) {
  reward.remaining = uint128(bal);
}

Recommended Mitigation Steps

bal should be decremented by the transfer amounts after transfers to the claimContract (and treasury), like how it’s done in the original contract.

if (reward.token == cvx || reward.token == crv) {
  uint256 treasuryFee = d_reward / 5;
  IERC20(reward.token).transfer(treasury, treasuryFee);
  bal -= treasuryFee;
  d_reward -= treasuryFee;
}
IERC20(reward.token).transfer(address(claimContract), d_reward);
bal -= dReward;

Lines of code

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

Vulnerability details

Impact

The lpSupply is derived from the Masterchef’s deposit token balance.

lpSupply = pool.depositToken.balanceOf(address(this));

However, it is important to note that there is no stake token transfer for deposits and withdrawals, so lpSupply() is expected to be zero (unless someone sends some deposit tokens to the contract). As a result, there is no reward accrual for all users.

// snippet from updatePool()
if (lpSupply == 0 || pool.allocPoint == 0) {
  pool.lastRewardBlock = block.number;
  return;
}

Recommended Mitigation Steps

Save lpSupply in the PoolInfo struct. Increment and decrement the variable for deposits / withdrawals. Note that the deposit fee should be excluded from the increment because it’s not credited to anyone.

struct PoolInfo {
  IERC20 depositToken; // Address of LP token contract.
  uint allocPoint; // How many allocation points assigned to this pool. to distribute per block.
  uint lastRewardBlock; // Last block number that distribution occurs.
  uint accConcurPerShare; // Accumulated per share, times multiplier. See below.
  uint lpSupply; // total amount of LP tokens supplied. Internal accounting is done because contract does not actually hold the LP tokens
  uint16 depositFeeBP; // Deposit fee in basis points
}

// TODO: replace line (or remove) below with pool.lpSupply
uint lpSupply = pool.depositToken.balanceOf(address(this));

// Eg. in pendingConcur()
accConcurPerShare = accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(pool.lpSupply));

function deposit() {
  ...
  if (_amount > 0) {
    if (pool.depositFeeBP > 0) {
      uint depositFee = _amount.mul(pool.depositFeeBP).div(_perMille);
      user.amount = SafeCast.toUint128(user.amount + _amount - depositFee);
      // depositFee is excluded because portion of reward would've been unaccounted for
      pool.lpSupply += _amount - depositFee;
    } else {
      user.amount = SafeCast.toUint128(user.amount + _amount);
      pool.lpSupply += _amount;
    }
  }
  ...
}

function withdraw() {
  ...
  if (_amount > 0) {
    user.amount = SafeCast.toUint128(user.amount - _amount);
    pool.lpSupply -= _amount;
  }
  ...
}

Lines of code

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

Vulnerability details

Impact

It is possible to add a pool with > 100% deposit fee. Given that there is no way to update the fee, and that a pool can only be registered once, an incorrect value set would cause deposits to fail.

Side note: the deposit fee is improperly handled (see other issue submitted).

Recommended Mitigation Steps

Have a sanity check to ensure the deposit fee is ≤ 100%.

function add(address _token, uint _allocationPoints, uint16 _depositFee, uint _startBlock) public onlyOwner {
	// note: depositFeeBP and _perMille are in different denominations, mentioned in separate issue
	require(_depositFee <= _perMille, "depositFee exceeds maximum value");
	...
}

Lines of code

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

Vulnerability details

Impact

The pool’s accConcurPerShare ceases to update once the block number exceeds the endBlock. This means that there is potentially a period between the last accounted block and end block that fails to update accConcurPerShare.

Proof of Concept

• Assume endBlock = 5000, pool.lastRewardBlock = 4900
• The next time updatePool() is called is when block.number = 5001
• The rewards for the last 100 blocks (4900 to 5000) are skipped.

Recommended Mitigation Steps

The 1st and 3rd if blocks in updatePool() can be combined.

function updatePool(uint _pid) public {
  PoolInfo storage pool = poolInfo[_pid];
  // take minimum of endBlock and current block number
  uint lastBlockToReward = endBlock > block.number ? block.number : endBlock;
  // only need to update state if pool.lastRewardBlock < lastBlockToReward
  if (lastBlockToReward <= pool.lastRewardBlock) return;
  ...
  // if(block.number >= endBlock) {
  //   pool.lastRewardBlock = block.number;
  //   return;
  // }
  ...
}

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

G01: Redundant variables / events

The following variables / events are initialized / declared, but their values are not used in the contract subsequently. Consider removing them.

Variables

Masterchef.startBlock

USDMPegRecovery.startLiquidity

Events

Masterchef.EmergencyWithdraw

G02: Immutable / constant variables

The following variables can be made immutable or constant.

USDMPegRecover.step

StakingRewards.rewardsToken

StakingRewards.stakingToken

Masterchef.concurPerBlock`

Masterchef.concur

Masterchef._concurShareMultiplier

Masterchef._perMille

G03: Masterchef: Redundant need for SafeMath

Solidity 0.8 comes with in-built integer flow checks. The use of SafeMath for uint is therefore unnecessary.

G04: Masterchef: Simplify safeConcurTransfer()

function safeConcurTransfer(address _to, uint _amount) private {
  uint concurBalance = concur.balanceOf(address(this));
  bool transferSuccess = false;
  if (_amount > concurBalance) {
      transferSuccess = concur.transfer(_to, concurBalance);
  } else {
      transferSuccess = concur.transfer(_to, _amount);
  }
	require(transferSuccess, "safeConcurTransfer: transfer failed");
}

can be simplified to

function safeConcurTransfer(address _to, uint _amount) private {
  uint concurBalance = concur.balanceOf(address(this));
	uint amountToTransfer = _amount >= concurBalance ? concurBalance : _amount;
	require(
		concur.transfer(_to, amountToTransfer),
		"safeConcurTransfer: transfer failed"
	);
}

G05: USDMPegRecovery: save contract’s usdm balance in provide()

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

Instead of fetching the contract’s USDM balance twice, it would save gas to store the result in a local variable and reuse it.

uint usdmBal = usdm.balanceOf(address(this));
require(usdmBal >= totalLiquidity.usdm, "<liquidity");
// truncate amounts under step
uint256 addingLiquidity = (usdmBal / step) * step;

G06: ConvexStakingWrapper: Replace IRewardStaking(extraPool).rewardToken() with extraToken

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

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

Since IRewardStaking(extraPool).rewardToken() has already been fetched once and saved in extraToken, gas can be saved by using it instead of fetching the value again.

rewards[_pid].push(
	RewardType({
	  token: extraToken,
	  pool: extraPool,
	  integral: 0,
	  remaining: 0
	})
);