Concur Finance contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Impact

The user can drain the contract.

Proof of Concept

The withdraw method does not check that the user has called this method before, there is also no tracking of how many tokens the user donates, so the calls client.shareOf(_token, msg.sender) / client.totalShare(_token ) are not related to how many tokens this user deposited, it is about how much balance was deposited.

This method does not subtract transferred tokens from savedTokens[_token], and also does not use the flag claimed[_token][_to] = true; so there is no restriction on a user calling this method multiple times until drain the contract.

Steps:

• User call withdraw multiple times

Recommended Mitigation Steps

• Use claimed[_token][_to] to avoid duplicate calls to withdraw

Lines of code

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

Vulnerability details

Impact

Shelter contract can steal user tokens.

Proof of Concept

Shelter client can call activate on an already activated token, this will reset its start time, so if the client activate a token when it GRACE_PERIOD is almost finished, it will reset this time. This will prevent the user to call withdraw because the condition activated[_token] + GRACE_PERIOD < block.timestamp but will allow the client to call deactivate and receive all funds from the users because it will satisfy the condition activated[_token] + GRACE_PERIOD > block.timestamp.

Steps:

• client activate tokenA.
• Users deposit tokenA using donate.
• client activate tokenA again until they has enough tokens.
• More users use donate.
• client deactivate tokenA and receive all tokens.

Recommended Mitigation Steps

• Avoid activate twice for the same token
• donate only after the GRACE_PERIOD

Non critical

Lack of events

The method ConcurRewardPool.pushReward should emit an event in order to be detectable by the _recipient.

Non exploitable reentrancy

The method ConcurRewardPool.claimRewards allow the reentrancy, it seems that it's not vulnerable but it should be protected in order to be resilient.

Low

Contract management risks

The following contracts are Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while paused should be avoided:

• ConvexStakingWrapper
• StakingRewards

contracts\ConvexStakingWrapper.sol

1. It was found some transfer, approve or transferFrom without checking the boolean result, ERC20 standard specify that the token can return false if this call was not made, so it's mandatory to check the result of these methods.

As following you can see the affected locations:

• ConvexStakingWrapper.sol#L179
• ConvexStakingWrapper.sol#L182

2. There are a lack of checks in the method requestWithdraw that allow to create a request for _amount=0 it should be denied in order to avoid possible errors.

General issues

1. Change the incremental logic from i++ to ++i in order to save some opcodes:

• ConvexStakingWrapper.sol#L121
• ConvexStakingWrapper.sol#L219
• ConcurRewardPool.sol#L35

2. Use delete instead of set to default value (false or 0)

• Shelter.sol#L46-L47
• StakingRewards.sol#L120
• MasterChef.sol#L83
• USDMPegRecovery.sol#L65
• ConcurRewardPool.sol#L38

Contracts specific

Contract contracts\MasterChef.sol

1. The following variables could be declared as constant:

• concurPerBlock
• _concurShareMultiplier
• _perMille

2. The following variables could be declared as immutable:

• startBlock
• endBlock
• concur

3. There was an empty pool in lines L63-L70, pushed during the constructor that never is used.

4. The following struct could be optimized moving depositFeeBP close to depositToken in order to save one storage slot:

struct PoolInfo {
        IERC20 depositToken; // <- Move here
        uint allocPoint;
        uint lastRewardBlock;
        uint accConcurPerShare;
        uint16 depositFeeBP; // <- Move this
    }

5. On line 204, the transferSuccess variable is init false, it is better to create the variable with the return to avoid this initialization.

Contract contracts\VoteProxy.sol

1. In line 21 it could be removed the == true in order to save some opcodes.

Contract contracts\USDMPegRecovery.sol

1. The variable startLiquidity is never used and can be removed or changed to immutable.
2. The variable step coud be constant.
3. On lines 74-76 usdm.balanceOf(address(this)) was called twice without any change of balance, it's cheaper to cache it.

Contract contracts\ConvexStakingWrapper.sol

1. In line 235 it's better to move the asignation of deposits[_pid][msg.sender].amount inside the if if (_amount > 0) {

deposits[_pid][msg.sender].amount -= uint192(_amount);
        if (_amount > 0) {