Concur Finance contest - gzeon's results

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L52

Vulnerability details

Impact

Shelter withdraw does not check if user already claimed. This allow any user with non-zero claim to drain the Shelter.

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L52

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

Recommended Mitigation Steps

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

Lines of code

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

Vulnerability details

Impact

After depositing into Curve, underlying USDM and pool3 of each LP share will change due to

1. Swap fee
2. Pool imbalance

However, userLiquidity does not update according to current underlying LP balance. When guardian remove liquidity from Curve, at least 1 of the token would have an increased balance. These balance cannot be claimed by anyone because sum of userLiquidity represent the old balance. If any of the token have a decreased balance (e.g. caused by pool imbalance), withdrawal would be served in first-come-first-served basis. Any gained tokens are stuck in the contract.

Proof of Concept

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

1. Alice deposit 100 USDM to the contract
2. Bob deposit 100 pool3 to the contract
3. Guardian add 100 USDM and 100 pool3 liquidity to Curve
4. Due to trading and pool imbalance, the LP shares are now worth 1000 USDM and 10 pool3
5. Guardian remove liquidity
6. The contract have 1000 USDM and 10 pool3
7. Alice withdraw 100 USDM successfully
8. Bob can only withdraw 10 pool3
9. The contract have 900 USDM stuck

Recommended Mitigation Steps

A share type accounting would be prefered, but can be complex considering there are 2 tokens.

Lines of code

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

Vulnerability details

Impact

In README.me:

USDM deposits are locked based on the KPI’s from carrot.eth

However, USDM deposits are also locked until guardian remove liquidity because there are no mechanism to remove deposited USDM in withdraw

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

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L38

Vulnerability details

Impact

In the current design, client cannot deactivate a token after GRACE_PERIOD passed (L45). However, since activate does not check if the token is activate, client can call activate to reset the timestamp and call deactivate immediately to steal all the donated token.

Proof of Concept

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L38

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

Recommended Mitigation Steps

        require(activated[_token] == 0, "already activated");

Rounding issue in ConvexStakingWrapper

rouding issue ConvexStakingWrapper might lead to revert https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L178

        if (reward.token == cvx || reward.token == crv) {
            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = (d_reward * 4) / 5;
        }
        IERC20(reward.token).transfer(address(claimContract), d_reward);

fix by changing to

        if (reward.token == cvx || reward.token == crv) {
            IERC20(reward.token).transfer(treasury, d_reward / 5);
            d_reward = d_reward - d_reward / 5;
        }
        IERC20(reward.token).transfer(address(claimContract), d_reward);

Lack input validation in constructor of ConvexStakingWrapper

masterChef can be set to address(0) which will require redeployment https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L71

Cast to uint192 is unsafe

Cast to uint192 is unsafe, user trying to deposit more than 2^192 would recevie less deposits balance. https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L235

        deposits[_pid][msg.sender].amount += uint192(_amount);

Lack input validation in constructor of ConcurRewardPool

rewardNotifier can be set to address(0) which will require redeployment https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConcurRewardPool.sol#L15

USDMPegRecovery withdraw does not check if user have sufficient balance

USDMPegRecovery withdraw does not check if user have sufficient balance. While it would revert later in the procedure due to an overflow when deducting from user.usdm/user.pool3, it is better to have the check at the begining. https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L110

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

Use struct reference instead of memory copy

A lot of pattern in the code is like

RewardType memory reward = rewards[_pid][_index];
// do something to reward
rewards[_pid][_index] = reward;

it might be better to use

RewardType storage reward = rewards[_pid][_index];
// do something to reward

gas-wise they should be the same, but using reference make it less likely to forget commit back to storage.

Typo

RADSs should be CONCURs https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L25

    // We do some fancy math here. Basically, any point in time, the amount of RADSs

No need to use SafeMath in Solidity >=0.8.0

Math is safe by default in Solidity >=0.8.0 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L14

    using SafeMath for uint;

Also need to replace SafeMath function to normal arithmetic in the MasterChef contract.

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590

$ grep -Rn "> 0" ./contracts/ | grep require
./contracts/MasterChef.sol:186:        require(user.amount > 0, "MasterChef: nothing to withdraw");
./contracts/StakingRewards.sol:94:        require(amount > 0, "Cannot stake 0");
./contracts/StakingRewards.sol:108:        require(amount > 0, "Cannot withdraw 0");

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/

Variables that could be set immutable

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

    uint256 public startLiquidity;
    uint256 public step;

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L19

    IERC20 public rewardsToken;
    IERC20 public stakingToken;
    address public rewardsDistribution;

Use uint256 instead of bool

Use uint(1) instead of bool(true) to save gas by avoiding masking ops https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L17

    mapping(IERC20 => mapping(address => bool)) public override claimed;

Cache block.timestamp + GRACE_PERIOD to activated[_token]

Since we are only interested in activated[_token] + GRACE_PERIOD, might as well cache it when we call activate. https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L38

    function activate(IERC20 _token) external override onlyClient {
        activated[_token] = block.timestamp + GRACE_PERIOD;
        savedTokens[_token] = _token.balanceOf(address(this));
        emit ShelterActivated(_token);
    }

    function deactivate(IERC20 _token) external override onlyClient {
        require(activated[_token] != 0 && activated[_token] > block.timestamp, "too late");
        activated[_token] = 0;
        savedTokens[_token] = 0;
        _token.safeTransfer(msg.sender, _token.balanceOf(address(this)));
        emit ShelterDeactivated(_token);
    }

    function withdraw(IERC20 _token, address _to) external override {
        require(activated[_token] != 0 && activated[_token] < 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);
    }

Unchecked safe math

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

            total.usdm += _deposits.usdm;
            user.usdm += _deposits.usdm;

can be changed to

            total.usdm += _deposits.usdm;
            unchecked{user.usdm += _deposits.usdm;}

assuming user.usdm always <= total.usdm

similarly

            total.pool3 += _deposits.pool3;
            user.pool3 += _deposits.pool3;

can be changed to

            total.pool3 += _deposits.pool3;
            unchecked{user.pool3 += _deposits.pool3;}

float multiplication optimization

// out = x * y unchecked{/} z
function fmul(uint256 x, uint256 y, uint256 z) internal pure returns(uint256 out){
assembly{
    if iszero(eq(div(mul(x,y),x),y)) {revert(0,0)}
    out := div(mul(x,y),z)
}
}

can be applied to floating multiplication such as in https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L180

            d_reward = (d_reward * 4) / 5;