Concur Finance contest - Rhynorater's results

Lines of code

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

Vulnerability details

Impact

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

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

In the above withdraw function in the Shelter contract, the claimed mapping is never checked to see if the msg.sender has already claimed their tokens already. As a result, the user can keep on claiming and claiming until all of the tokens are stolen. Furthermore, the _to parameter is used in the claimed[_token][_to] = true; definition. This allows the attacker to claim their own token amount under another EOA's address. An attacker with money locked in the shelter can generate an infinite amount of EOAs and call the withdraw function with those addresses in the _to field from the attacker's address and steal all the tokens since the _to is changing every time and the msg.sender is kept consistent.

Tools Used

Manual analysis

Recommended Mitigation Steps

I'd recommend removing the _to variable and using msg.sender instead. Additionally, the claimed mapping should be checked to see if the user has already claimed their tokens.

Lines of code

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

Vulnerability details

Impact

The CEI pattern is not being implemented properly in the claimRewards function of the ConcurRewardPool.sol. https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L37

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

On line 37, the safeTransfer method is called on the ERC20 token (note, safeTransfer does not protect against re-entrancy attacks) and THEN the reward value is set to 0. If this token is question is an ERC777 token with callbacks (this is possible as ERC777 is backward compatible and both Convex's VirtualBalanceRewardPool.sol and Concur's ConvexStakingWrapper.sol utilize IERC20 to interact with the token) then the attacker can utilize the callback to re-enter the function and drain the contract of rewards before the reward is set to 0.

This attack vector would also be present if future updates to the code enabled rewards to be automatically swapped to ETH and the same pattern was followed.

Tools Used

Manual review

Recommended Mitigation Steps

Follow the CEI pattern.

Missing 0 Address Check

Severity: Low https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L16

    constructor(address _notifier) {
        rewardNotifier = _notifier;
    }

The rewardNotifier is a crucial parameter that should be zero address checked.

Missing input validation for _amount in deposit and withdraw

Severity: Low https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L236 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L262

The _amount parameter to the withdraw and deposit functions should be checked not to be zero. Otherwise, the following if statements:

        if (_amount > 0) {

Will not trigger, allowing an user to accidentially reset their deposit or withdraw request epoch and lock funds for another cycle. This can also lead to the spam of events at the end of these functions which may cause issues with logging.

Use of msg.sender and _msgSender can result in wasted gas

Severity: Low/Gas Depositors may look the the body of the MasterChef.sol#withdraw and MasterChef.sol#deposit function and note that it is possible to use meta transactions to interact with this contract due to the user of _msgSender. However, in the onlyDepositor modifier, msg.sender is used which would be the relayer, not the meta transaction originator. This may result in user confusion and frustration or wasted gas if the meta transaction functionality was not necessary.

Public function should be External

Severity: Gas Since the following functions are not called from within their own contract, the visibility quantifier shoudl be set to external to save on gas. https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L93

Cache calls to External Contracts

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L93-L99

    function addRewards(uint256 _pid) public {
        address mainPool = IRewardStaking(convexBooster)
            .poolInfo(_pid)
            .crvRewards;
        if (rewards[_pid].length == 0) {
            pids[IRewardStaking(convexBooster).poolInfo(_pid).lptoken] = _pid;
            convexPool[_pid] = mainPool;

On ConvexStakingWrapper.sol#L94-85 there is a call made to RewardStaking(convexBooster).poolInfo(_pid). This makes an external call to the convexBooster contract. This same call is then performed just a couple lines down on line 98. Caching the result of this would reduce gas usage significantly for this function.

Avoid Interactions with External Contracts Where Possible

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L209-L222

    function _checkpoint(uint256 _pid, address _account) internal {
        //if shutdown, no longer checkpoint in case there are problems
        if (paused()) return;

        uint256 supply = _getTotalSupply(_pid);
        uint256 depositedBalance = _getDepositedBalance(_pid, _account);

        IRewardStaking(convexPool[_pid]).getReward(address(this), true);

        uint256 rewardCount = rewards[_pid].length;
        for (uint256 i = 0; i < rewardCount; i++) {
            _calcRewardIntegral(_pid, i, _account, depositedBalance, supply);
        }
    }

In the _checkpoint function, if a user does not have any depositedBalance, then they will not have any rewards that need to be issued. This is true since when a user calls the withdraw function (the only function which decreases the user's deposited balance) that function will perform a _checkpoint call which will transfer the user's rewards to be claimed. As such, the deposit amount cannot ever be 0 and there still be rewards left to be claimed.

As such, adding an if statement check whether there is a depositedBalance before reaching out to the convexPool will save the initial gas cost when first creating a deposit, which will decrease the friction of using Concur for the first time.

Unchecked Math

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L261 It is possible to put ConvexStakingWrapper.sol#L261 inside the unchecked directive. This is due to the fact that on line 259 we see that the request.amount much be greater-than or equal to the requested _amount. This request.amount is limited by the deposits[_pid][msg.sender].amount in the requestWithdraw function. The only way for the deposits[_pid][msg.sender].amount to decrease is via the withdraw function. This withdraw function also deletes the withdrawRequest on completion and is nonReentrant. As a result, it is impossible for _amount to be greater than deposits[_pid][msg.sender].amount.

However, if you do decide to make this code optimization, I'd recommend commenting around it explaining the above as it does add complexity to the code and a modification elsewhere could turn this into a vulnerability.