Concur Finance contest - GalloDaSballo's results

Lines of code

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

Vulnerability details

Impact

Shelter.withdraw seem to allow for withdrawal of rescued funds

the function uses claimed[_token][_to] = true; to signal that the user already withdrew

However there's no check at the top of the function to avoid to to call this function again.

This means each user can call it multiple times, each time getting more tokens out

Tools Used

Recommended Mitigation Steps

Add a check at the beginning of the function

    function withdraw(IERC20 _token, address _to) external override {
        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
       require(!claimed[_token][_to], "Already claimed");
        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);
    }

Lines of code

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

Vulnerability details

Impact

In USDMPegRecovery deposit and withdraw allow for direct deposits of a specific token (3crv or usdm)

The balances are directly changed and tracked in storage.

provide seems to be using the real balances (not the ones store) to provide liquidity. Because of how curve works, you'll be able (first deposit) to provide exactly matching liquidity. But after (even just 1 or) multiple swaps, the pool will be slightly imbalanced, adding or removing liquidity at that point will drastically change the balances in the contract from the ones tracked in storage.

Eventually users won't be able to withdraw the exact amounts they deposited.

This will culminate with real balances not matching user deposits, sometimes to user advantage and other times to user disadvantage, ultimately to the protocol dismay.

Proof of Concept

Deposit equal usdm and 3crv LP Do one trade on CRV Withdraw the LP

The real balances are not matching the balances in storage

User tries to withdraw all their balances, inevitable revert

Recommended Mitigation Steps

Either find a way to price the user contribution based on the LP tokens (use virtual_price) Or simply have people deposit the LP token directly (avoiding the IL math which is a massive headache)

Lines of code

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

Vulnerability details

Impact

StakingRewards.recoverERC20 rightfully checks against the stakingToken being sweeped away. However there's no check against the rewardsToken which over time will sit in this contract.

This is the case of an admin privilege, which allows the owner to sweep the rewards tokens, perhaps as a way to rug depositors

Proof of Concept

calling StakingRewards.recoverERC20(rewardsToken, rewardsToken.balanceOf(this)) enables the owner to sweep the token

Recommended Mitigation Steps

Add an additional check

        require(
            tokenAddress != address(rewardsToken),
            "Cannot withdraw the rewards token"
        );

Lines of code

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

Vulnerability details

Impact

Because the code get's the reward amount, then transfers and lastly set's the value to 0, the code is vulnerable to re-entrancy.

This is contingent on using either a malicious token or a token with hooks (see ERC777 or tokens with notifyTransfer)

The re-entrancy would happen in the following way, and assuming a malicious / vulnerable token would allow for the draining of all rewards (all funds in the contract)

1. call claimRewards([VULNERABLE_TOKEN])
2. Receive x tokens, re-enter here
3. call again claimRewards([VULNERABLE_TOKEN])
4. Receive x tokens again, loop until contract is drained
5. After the re-entrancy exploit the contract sets storage to 0

I've run through alternative scenarios where I use a user created token with the goal of draining funds, but don't believe that is possible, as such have rated medium (it's contingent on either using a token with hooks)

Proof of Concept

1. call claimRewards([VULNERABLE_TOKEN])
2. Receive x tokens, re-enter here
3. call again claimRewards([VULNERABLE_TOKEN])
4. Receive x tokens again, loop until contract is drained
5. After the re-entrancy exploit the contract sets storage to 0

Recommended Mitigation Steps

Set the storage to 0, then do the transfer

Change to

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