QA report

[code423n4]

Handle

wuwe1

Vulnerability details

Potential reentrance in claimRewards

POC

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

            IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
            reward[msg.sender][_tokens[i]] = 0;

Considering there are exterTokens, it is possible that some token will provide reentry opportunities.

Mitigation

change to order of L37 and L38

            reward[msg.sender][_tokens[i]] = 0;
            IERC20(_tokens[i]).safeTransfer(msg.sender, getting);

Potential out of gas in massUpdatePools

POC

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

    function massUpdatePools() public {
        uint length = poolInfo.length;
        for (uint _pid = 0; _pid < length; ++_pid) {
            updatePool(_pid);
        }
    }

Mitigation

change to

function massUpdatePools(uint256[] calldata pids) external {
    uint256 len = pids.length;
    for (uint256 i = 0; i < len; ++i) {
        updatePool(pids[i]);
    }
}

Left dust CRV & CVX in ConvexStakingWrapper

POC

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

            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = (d_reward * 4) / 5;

Current implementation will always left 1 CRV or CVX in the contract.

Mitigation

uint256 toTreasuryAmount = d_reward / 5;
IERC20(reward.token).transfer(treasury, toTreasuryAmount);
d_reward = d_reward - toTreasuryAmount;

should use safeTransfer

These two call are not using safeTransfer

Several tokens do not revert in case of failure and return false.

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

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

Lacking event

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

add has no event, so it is difficult to track off-chain changes in the poolInfo.

consider add a event like

https://github.com/sushiswap/sushiswap/blob/canary/contracts/MasterChefV2.sol#L123

[GalloDaSballo]

Potential reentrance in claimRewards Dup of #86 (Med)

Potential out of gas in massUpdatePools Doesn't actually solve as you can still run out of gas in an external call, and, for other sponsor mistakes the function is unused

Left dust CRV & CVX in ConvexStakingWrapper Valid

should use safeTransfer Agree

Lacking event Non-critical

[GalloDaSballo]

Adding #83 to this, the warden has shown how dust amounts can accrue over time to the vault

[GalloDaSballo]

3+

[GalloDaSballo]

Bumped to 4 to be 2nd best submission

[JeeberC4]

@CloudEllie please create new issue for the Med finding above.

[CloudEllie]

Created separate issue for "Potential reentrance in claimRewards" - see #269