Concur Finance contest - peritoflores's results

ClaimRewards is vulnerable to reentrancy

This function claimRewards#ConcurRewardPool.sol is vulnerable to reentrancy though no token compromised this can be very dangerous because it is a critical function.

function claimRewards(address[] calldata _tokens) external override { 
    for (uint256 i = 0; i < _tokens.length; i++) {
        uint256 getting = reward[msg.sender][_tokens[i]];
        IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
        reward[msg.sender][_tokens[i]] = 0;
    }
}

The trick is the following: As _tokens is controlled by an attacker it is possible to deploy a IERC20 contract with the function transfer() overriden and from there call back claimReward or (other function). Even your are using safeTransfer from OpenZeppelin SafeERC library this function will call transfer at low level.

#PoC

As a proof of concept I share this code that you can easily test in remix

pragma solidity ^0.8.0;

 import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol";
import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol";

 contract EvilERC20 is IERC20{

function totalSupply() external view override returns (uint256){
    return 0;
}
    event EvilMessage(string message);


function balanceOf(address account) external view override returns (uint256){
    return 0;
}


function transfer(address recipient, uint256 amount) external override returns (bool){
    emit EvilMessage("I am evil and I can perform reentrancy attack");
    return true;
}


function allowance(address owner, address spender) external override view returns (uint256){
    return 0;
}


function approve(address spender, uint256 amount) external override returns (bool){
    return true;
}


function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool){
    return true;
}


}


contract TestReentracyinsafeTransfer{

using SafeERC20 for IERC20;
IERC20 myToken;

constructor(){
    myToken= new EvilERC20();
}


function test() public {
    myToken.safeTransfer(msg.sender, 0);

}
}

`

If you call test#TestReentracyinsafeTransfer even your are using safeERC20 the event "I am evil and I can perform reentrancy attack" will be triggered

Proposed solution

I believe that you should at least put a comment here because if at some point you increment a variable in that function the exploit would work. I will not harm to add nonReentrant modifier.

Finally also I recommend you switch the order

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

It is always safer to set a variable to 0 and then call the external contract.

Lines of code

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

Vulnerability details

Problem

You are assuming that ERC20.transfer() always returns true, however according to the EIP-20 Standard (https://eips.ethereum.org/EIPS/eip-20)

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

There is also a known issue that many token do not implement well this transfer function so the most recommendable solution in this situation is to use safeTransfer from safeERC20 that you already imported in your file. Similar reports at https://github.com/code-423n4/2021-08-notional-findings/issues/68

Impact

Reward will not be properly transfer to treasury or claimContract.

Remove SafeMath from most gas consuming functions

Now you are using solidity 0.8 you can remove SafeMath .

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L119-L121

The following is important I believe as you mention massUpdatePools() consumes lot of gas

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L151-L152