Concur Finance contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Shelter.withdraw allows msg.sender to withdraw to arbitrary to address and the only state update is claimed[_token][_to] = true, while claimed isn't checked on the entrance.

This way msg.sender can withdraw again and again, emptying contract token holdings.

Proof of Concept

Shelter.withdraw doesn't check or update a state for msg.sender on a withdrawal:

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

Recommended Mitigation Steps

Even if withdraw() would require claimed[_token][_to] == false it will not work as to address is arbitrary, so the withdrawals can be repeated with another to addresses.

This way, the simplest recommended solution is to mark the flag for msg.sender and check it for the msg.sender as well.

Now:

require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
...
claimed[_token][_to] = true;

To be:

require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
require(!claimed[_token][msg.sender], "already claimed");
...
claimed[_token][msg.sender] = true;

Another solution is to use a client for the record keeping, but this requires state translation as well: now only read-only client.shareOf(_token, msg.sender) and client.totalShare(_token) are being called and no msg.sender flags set, i.e. the state isn't updated, the client can't know that msg.sender withdrew already.

Lines of code

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

Vulnerability details

Impact

notifyRewardAmount will be inoperable if rewardsDuration bet set to zero. If will cease to produce meaningful results if rewardsDuration be too small or too big

Proof of Concept

The setter do not control the value, allowing zero/near zero/enormous duration:

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

Division by the duration is used in notifyRewardAmount:

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

Recommended Mitigation Steps

Check for min and max range in the rewardsDuration setter, as too small or too big rewardsDuration breaks the logic

Lines of code

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

Vulnerability details

Impact

If deposits and withdraws are done frequently enough, the reward update operation they invoke will deal mostly with the case when there is nothing to add yet, i.e. reward.remaining match the reward token balance.

If reward token doesn't allow for zero value transfers, the reward update function will fail on an empty incremental reward transfer, which is now done unconditionally, reverting the caller deposit/withdrawal functionality

Proof of Concept

When ConvexStakingWrapper isn't paused, every deposit and withdraw update current rewards via _checkpoint function before proceeding:

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

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

_checkpoint calls _calcRewardIntegral for each of the reward tokens of the pid:

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

_calcRewardIntegral updates the incremental reward for the token, running the logic even if reward is zero, which is frequently the case:

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

If the reward token doesn't allow zero value transfers, this transfer will fail, reverting the corresponding deposit or withdraw

Recommended Mitigation Steps

Consider checking the reward before doing transfer (and the related computations as an efficiency measure):

Now:

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

To be:

if (d_reward > 0)
	IERC20(reward.token).transfer(address(claimContract), d_reward);

1. ConvexStakingWrapper constructor and core configuration setters don't check the values (low)

Impact

A range of malfunctions is possible if core configuration parameters are set by mistake to any values that make little sense. It is also not always right away evident that such a mistake took place

Proof of Concept

Setters don't check the core configuration values:

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

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

Recommended Mitigation Steps

Consider adding zero address, zero and range parameter checks

2. Unlocked pragma (low)

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced

Proof of Concept

Contracts use pragma solidity ^0.8.11:

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

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

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

Recommended Mitigation Steps

Fix the compiler version across the system

3. Pool3 balance isn't checked before liquidity provision (non-critical)

Impact

System then will fail with low level error message on liquidity addition attempt

Proof of Concept

pool3 balance isn't checked before liquidity provision, looks like it's assumed to always match usdm balance:

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

Recommended Mitigation Steps

Check both balances before calling add_liquidity

4. Use custom errors to save gas (low)

Impact

Code is less clear, gas is overspent on requires

Proof of Concept

solidity 0.8.11 allows for custom errors:

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

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

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

Recommended Mitigation Steps

Consider introducing and using custom errors instead of the revert strings to save gas:

https://blog.soliditylang.org/2021/04/21/custom-errors/