Concur Finance contest - csanuragjain's results

Handle

csanuragjain

Vulnerability details

Impact

Funds can be stolen by calling withdraw again and again

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol

2. Observe the withdraw function

function withdraw(IERC20 _token, address _to) external override {
        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
        uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);
        claimed[_token][_to] = true;
        emit ExitShelter(_token, msg.sender, _to, amount);
        _token.safeTransfer(_to, amount);
    }

3. Observe that this function does not check if user has already claimed the amount and neither it reduces user shares which means client.shareOf(_token, msg.sender) is never reduced. User can exploit it by calling withdraw repeatedly and drain contract funds

Recommended Mitigation Steps

Change withdraw function like below:

function withdraw(IERC20 _token, address _to) external override {

require(!claimed[_token][_to] , "Already claimed");

        ...
        claimed[_token][_to] = true;
        ...
    }

Handle

csanuragjain

Vulnerability details

Impact

Deposit and withdraw are done over msg.sender instead of _recipient

Proof of Concept

1. Navigate to contract https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol

2. Observe the deposit and withdraw function

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

3. Observe user is initialized with msg.sender instead of _recipient for whom deposit and withdraw is called

Recommended Mitigation Steps

Change UserInfo storage user to

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

Handle

csanuragjain

Vulnerability details

Impact

onlyClient can deactivate a token even after deadline is passed and transfer all token balance to itself

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol

2. Observe that token can only be deactivated if activated[_token] + GRACE_PERIOD > block.timestamp. We will bypass this

3. onlyClient activates a token X using the activate function

4. Assume Grace period is crossed such that activated[_token] + GRACE_PERIOD < block.timestamp

5. Now if onlyClient calls deactivate function, it fails with "too late"

6. But onlyClient can bypass this by calling activate function again on token X which will reset the timestamp to latest in activated[_token] and hence onlyClient can now call deactivate function to disable the token and retrieve all funds present in the contract to his own address

Recommended Mitigation Steps

Add below condition to activate function

function activate(IERC20 _token) external override onlyClient {
require(activated[_token]==0, "Already activated");
        activated[_token] = block.timestamp;
        savedTokens[_token] = _token.balanceOf(address(this));
        emit ShelterActivated(_token);
    }

Handle

csanuragjain

Vulnerability details

Impact

User rewards are lost

Proof of Concept

1. Navigate to contract at https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol

2. Observe the updatePool function

3. Lets say User deposited some amount when block.number = endBlock-x, this will set pool.lastRewardBlock=endBlock -x

4. Now assume next activity on this contract happens when block.number = endBlock+y, say User makes a deposit which triggers updatePool

5. updatePool will simply return without updating accConcurPerShare since

if(block.number >= endBlock) {
            pool.lastRewardBlock = block.number;
            return;
        }

6. But this is incorrect since contract is now not giving reward for duration x

Recommended Mitigation Steps

Do not return if block.number >= endBlock instead make pool.lastRewardBlock = endBlock. Also change getMultiplier such that

function getMultiplier(uint256 _from, uint256 _to) public view returns (uint256) {
        if (_to <= endBlock) {
            return _to.sub(_from);
        } else if (_from >= endBlock) {
            return 0;
        } else {
            return endBlock.sub(_from);
        }
    }

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
        }

Handle

csanuragjain

Vulnerability details

Impact

Gas savings

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol claimRewards function : use ++i instead of i++ in loop claimRewards function : Save _tokens[i] in local variable and then use the local variable within loop claimRewards function : Add check require(getting>0, "No reward left for token "+_tokens[i]) after line 36

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol withdraw function : claimed[_token][_to] is never used

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol withdraw function : Add check to see user balance is enough - require(user.usdm>=_withdrawal.usdm && user.pool3>=_withdrawal.pool3,"Insuffiencient balance")

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol pendingConcur function : Condition is missing for allocPoint being 0 - Change if (block.number > pool.lastRewardBlock && lpSupply != 0) to if (block.number > pool.lastRewardBlock && lpSupply != 0 && pool.allocPoint != 0) pendingConcur/deposit/withdraw function : Add a check require(pid<=poolInfo.length - 1) preventing invalid pid deposit/withdraw function : Add check require(_amount>0) preventing further actions

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol rewardPerToken function : Add below condition so that function reverts - if(lastTimeRewardApplicable() <= lastUpdateTime){return rewardPerTokenStored;}

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol deposit/withdraw function : add a condition to revert if amount=0