QA Report

[code423n4]

Handle

csanuragjain

Vulnerability details

Low findings

---

1. Incorrect Condition Finding:

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

In deactivate function, deactivation is rejected even if activated[_token] + GRACE_PERIOD = block.timestamp even when it should be allowed till Grace period.

Remediation: This should be corrected by changing require condition to below:

require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD >= block.timestamp, "too late");

2. Zero Address check missing

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

In withdraw function, Zero address checks can be added for to address which can prevent losses

Remediation:

require(to!=address(0), "Incorrect address");

3. User fund stuck

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

In withdraw function, Funds will stuck if user deposited a amount and then isDepositor[_depositor] is set to false by Admin. Now user cannot withdraw the amount since onlyDepositor will fail

Remediation: Withdraw should be independent of onlyDepositor

4. Incorrect PID updation

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

In massUpdatePools function, pid 0 should not be updated as poolInfo[_pid] is filled by dummy zero address token in constructor

constructor(IERC20 _concur, uint _startBlock, uint _endBlock) Ownable() {
        startBlock = _startBlock;
        endBlock = _endBlock;
        concur = _concur;
        poolInfo.push(
            PoolInfo({
            depositToken: IERC20(address(0)),
            allocPoint : 0,
            lastRewardBlock : _startBlock,
            accConcurPerShare : 0,
            depositFeeBP : 0
        }));
    }

Remediation: The loop in massUpdatePools function should start with value 1 instead of 0

5. Insecure transfer method used

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

In safeConcurTransfer function, transfer function is used

Remediation: use safeTransfer instead of transfer which is more secure

6. User funds can be added to 0 or non-existent pid with 0 rewards

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

a. In deposit & withdraw function, both accepts 0 pid which is zero address pool added by constructor, which means user funds would get added to incorrect pool.

b. Similarly non existent pid will also be accepted by both of these functions.

c. Offcourse these incorrect pid will not incur any reward since pool.accConcurPerShare will always be 0 which means user amount is added to a pool without any reward

Remediation: Add a check to see if pid>0 and pid<poolInfo.length

7. Extra reward will be given to users

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

In notifyRewardAmount function, Since block.timestamp value will change slightly while calculating periodFinish (when compared to lastUpdateTime) so periodFinish will actually become lastUpdateTime+rewardsDuration+x which is incorrect and would impact rewardPerToken by making it slightly higher

lastUpdateTime = block.timestamp;
        periodFinish = block.timestamp + rewardsDuration;

Remediation: Store block.timestamp locally and then use local variable to update periodFinish and lastUpdateTime

8. Missing condition:

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

In addRewards function, it is not checked if extraToken is crv. Only cvx check is present

Remediation: Add below check

if (extraToken == crv) {rewards[_pid][CRV_INDEX].pool = extraPool;}

9. rewards entries are made even for incorrect/non-existent pid

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

Non existent pid will create crv and cvx entries for rewards[_pid] since rewards[_pid].length == 0

Remediation: Revert if mainPool is zero address, require (mainPool!=address(0));

Non critical findings

10. Missing events

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

In claimRewards function observe that No emit event is fired after successful reward claim by user. Ideally a new event should be triggered showing that reward was claimed successfully by user

11. Reward lost

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

In notifyRewardAmount function, if Admin added a reward 100 once block.timestamp >= periodFinish. Now if Admin decides to add 200 rewards calling this function at block.timestamp >= periodFinish then contract considers total reward as 200 and discards the reward 100 added initially

if (block.timestamp >= periodFinish) {
            rewardRate = reward / rewardsDuration; // old reward is not considered
        }

[GalloDaSballo]

Incorrect Condition Finding: Imprecise by 1 second valid but non-critical

Zero Address check missing Fair

User fund stuck Dup of #238 (med)

Incorrect PID updation I don't believe this has any impact beside wasting gas

Insecure transfer method used Agree

User funds can be added to 0 or non-existent pid with 0 rewards Technically this can happen only in niche cases, in lack of impact I agree with non-critical

Extra reward will be given to users TX happens in the same block at the same time, there is no passing of time during a transaction, have to disagree with this one

Missing condition: Not sure if done on purpose by the sponsor but interesting finding

rewards entries are made even for incorrect/non-existent pid I think this is mostly an oddity of the system but has no actual impact

Reward lost Dup of #107 (med)

[GalloDaSballo]

2+++++

[JeeberC4]

@CloudEllie please create 2 new issue for the Med findings above.

[JeeberC4]

Generating QA Report for warden, preserving original title: Non critical & Low findings

[CloudEllie]

Created separate issues for upgraded findings: #267 and #268