Concur Finance contest - IllIllI's results

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L90-L108 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L110-L128

Vulnerability details

Curve.fi pools charge fees when adding or removing liquidity. The only time fees are not applied are when withdrawals are done using remove_liquidity(). USDMPegRecovery keeps track of tokens deposited and withdrawn, but does not keep track of fees assessed on these operations. The pool in use by this token assesses a 0.040% fee as well as a 50.000% of 0.040% admin fee.

Impact

A whale, or a user/miner using same-block cross-transaction flashloans, can repeatedly deposit and withdraw a large number of tokens, which causes large dollar-value fees to be assessed by the pool, but the number of tokens the user gets is unaffected. The attacker can do this repeatedly to drain all contract funds.

Proof of Concept

These are two separate instances of the issue, as the attacker only needs to exploit one of the functions, then wait for the guardian to call add_liquidity() or remove_liquidity(). The exploits can be applied separately to each of the two tokens.

    function deposit(Liquidity calldata _deposits) external {
        Liquidity memory total = totalLiquidity;
        Liquidity memory user = userLiquidity[msg.sender];
        if(_deposits.usdm > 0) {
            usdm.safeTransferFrom(msg.sender, address(this), uint256(_deposits.usdm));
            total.usdm += _deposits.usdm;
            user.usdm += _deposits.usdm;
        }

        if(_deposits.pool3 > 0) {
            require(totalLiquidity.usdm > 4000000e18, "usdm low");
            pool3.safeTransferFrom(msg.sender, address(this), uint256(_deposits.pool3));
            total.pool3 += _deposits.pool3;
            user.pool3 += _deposits.pool3;
        }
        totalLiquidity = total;
        userLiquidity[msg.sender] = user;
        emit Deposit(msg.sender, _deposits);
    }

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L90-L108

    function withdraw(Liquidity calldata _withdrawal) external {
        Liquidity memory total = totalLiquidity;
        Liquidity memory user = userLiquidity[msg.sender];
        if(_withdrawal.usdm > 0) {
            require(unlockable, "!unlock usdm");
            usdm.safeTransfer(msg.sender, uint256(_withdrawal.usdm));
            total.usdm -= _withdrawal.usdm;
            user.usdm -= _withdrawal.usdm;
        }

        if(_withdrawal.pool3 > 0) {
            pool3.safeTransfer(msg.sender, uint256(_withdrawal.pool3));
            total.pool3 -= _withdrawal.pool3;
            user.pool3 -= _withdrawal.pool3;
        }
        totalLiquidity = total;
        userLiquidity[msg.sender] = user;
        emit Withdraw(msg.sender, _withdrawal);
    }

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L110-L128

Tools Used

Code inspection

Recommended Mitigation Steps

Keep track of fees and reduce deposits/withdrawals accordingly

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L243 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L267 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L121 https://github.com/code-423n4/2022-02-concur/blob/02d286253cd5570d4e595527618366f77627cdaf/contracts/StakingRewards.sol#L174 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L37 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L57

Vulnerability details

Some ERC20 tokens, such as USDT, allow for charging a fee any time transfer() or transferFrom() is called. If a contract does not allow for amounts to change after transfers, subsequent transfer operations based on the original amount will revert() due to the contract having an insufficient balance.

Impact

The current implementation does not work properly with fee-on-transfer tokens when depositing/withdrawing lp tokens, depositing/withdrawing rewards, and withdrawing tokens from the shelter, whereas the code from which these changes were forked is able to handle them.

Proof of Concept

The following calls are separate instances of this issue where the call may suddenly start reverting for a fee-on-transfer token when the fee is turned on right before the function call, or has bee on prior to the call: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L243 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L267 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L121 https://github.com/code-423n4/2022-02-concur/blob/02d286253cd5570d4e595527618366f77627cdaf/contracts/StakingRewards.sol#L174 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L37 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L57

Tools Used

Code inspection

Recommended Mitigation Steps

One way to mitigate the issue is to measure the contract's balance right before and after the asset-transferring functions.

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L34-L40

Vulnerability details

Some ERC20 tokens have code that lets their users register callbacks to be triggered when that user's account has the token sent to it. One such standard is ERC777 which is backwards-compatible with ERC20 code. An example of an ERC777 token is imBTC, which was used in the dForce hack where the attacker was able to use reentrancy to drain $25 million. This vulnerability also involves tokens that support recipient callback and involves reentrancy, but does not allow the hacker to exfiltrate more than rewards pushed but not claimed, in a specific pool that has a reward token with callbacks.

Impact

Using a reentrancy bug, an attacker is able to drain the rewards already pushed, but not claimed by reward recipients, to the ConcurRewardPool, for any pool whose reward token has recipient callbacks. I've marked this as 'medium' because there are not currently any pools with ERC777 token rewards, though users can create such pools at any time by creating a factory pool.

Proof of Concept

    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;
        }
    }

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L34-L40

The reward map for the msg.sender is only set to zero after safeTransfer() is called. If _tokens[i] is an ERC777 token, the attacker can register a recipient callback which calls claimRewards() recursively, allowing the attacker to drain all of the _tokens[i] that the contract holds. Because the ConcurRewardPool holds all rewards, rather than segregating them per-pool or per-account, the attacker is able to drain the rewards of a specific token, for all pools and all accounts. The attacker is able to do the attacker each time he/she is given a reward, and can choose to execute it or not, based on monitoring the blockchain for Withdraw events emitted for MasterChef for pools of interest.

Tools Used

Code inspection

Recommended Mitigation Steps

Set the reward to zero before calling safeTransfer()

Title: Public function massUpdatePools() can be broken by a malicious token

If a malicious user adds a factory pool with a custom erc20 token where the token reverts if any of its functions are called from the MasterChef address, any time this public function is called it will revert.

Impact

The public function can be made to revert every time, which impacts the function of the protocol. I'm not sure if this is Low or Medium, so I'm submitting this as my QA report and leaving it up to the judge. Please elaborate on the rationale so I can tag future reports correctly.

Proof of Concept

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

    // Update reward variables of the given pool to be up-to-date.
    function updatePool(uint _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        if (block.number <= pool.lastRewardBlock) {
            return;
        }
        uint lpSupply = pool.depositToken.balanceOf(address(this));
        if (lpSupply == 0 || pool.allocPoint == 0) {
            pool.lastRewardBlock = block.number;
            return;
        }
        if(block.number >= endBlock) {
            pool.lastRewardBlock = block.number;
            return;
        }        

        uint multiplier = getMultiplier(pool.lastRewardBlock, block.number);
        uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
        pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
        pool.lastRewardBlock = block.number;
    }

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

The balanceOf() function can be made to revert every time, which means the mass update will always fail. Any individual pool with a malicious token has its own updatePool() calls broken.

Tools Used

Code inspection

Recommended Mitigation Steps

Use try-catch when iterating over all pools in a single function

> 0 costs more gas than != 0 when use in a require() statement. The project has the following instances of this issue: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L186 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L94 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L108