Concur Finance contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The Shelter.withdraw() function doesn't check whether the caller has already claimed their funds. Thus a user can withdraw as many times as they want.

Proof of Concept

The withdraw() function only checks whether the requested token is activated and whether the request is made in a specific period: https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L53

It doesn't verify whether the user has already claimed. Thus, the user can call it multiple times to withdraw more funds than they are eligible to: https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L54-L57

Tools Used

none

Recommended Mitigation Steps

The function modifies the claimed map which seems to be used to represent who has already claimed their funds. But, the map isn't used anywhere else. Also, it sets the _to address as the one that claimed. But, that should be the msg.sender since the msg.sender is the one who has access to funds. Otherwise, the user could call withdraw() multiple times and use a different _to parameter.

So add the following:

    function withdraw(IERC20 _token, address _to) external override {
        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
        require(!claimed[_token][msg.sender], "already claimed");
        uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);

        claimed[_token][msg.sender] = true;
        emit ExitShelter(_token, msg.sender, _to, amount);
        _token.safeTransfer(_to, amount);
    }

Report

• Non-Critical
  • emit events for address or important state changes
  • don't convert to a smaller bit uint without verifying its value
• Low
  • use SafeERC20 or check return value of ERC20 functions
  • Shelter donations result in wrong internal balance if fee-on-transfer token is used
  • Shelter can delay the user's ability to withdraw their funds indefinitely

Non-Critical

emit events for address or important state changes

Stuff like a guardian being added to the contract or the treasury address being changed. All of that should be accompanied by an event.

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L60-L66
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L83
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L87
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L70

don't convert to a smaller bit uint without verifying its value

When converting to a smaller bit uint it might result in a smaller number if the value is higher than the maximum value of that uint type. Before converting it check whether the passed value is small enough.

require(x <= type(uint192).max);
uint192 y = uint192(x);

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L235

It shouldn't really be an issue here tho and since I don't expect anybody to have more than 2 ** 196 - 1 tokens to abuse this.

Low

use SafeERC20 or check return value of ERC20 functions

Some tokens don't revert if a transfer fails. Instead they just return false. Since the ConvexStakingWrapper contract potentially works with arbitrary ERC20 tokens it's recommended to use SafeERC20 when working with it. Or at least check the return values:

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L179
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L182

Shelter donations result in wrong internal balance if fee-on-transfer token is used

In case of a fee-on-transfer ERC20 token the savedTokens balance will not represent the actual balance of the contract:

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L34-L35

You can either disable the use of fee-on-transfer tokens for donations or use the actual balance:

// disable
uint oldBalance = _token.balanceOf(address(this));
_token.safeTransferFrom(msg.sender, address(this), _amount);
uint newBalance = _token.balanceOf(address(this));
require(_amount == (newBalance - oldBalance));
savedTokens[_token] += _amount;

// use actual balance
_token.safeTransferFrom(msg.sender, address(this), _amount);
savedTokens[_token] = _token.balanceOf(address(this));

Shelter can delay the user's ability to withdraw their funds indefinitely

By repedeatly calling the activate() function for a specific token they can extend the timestamp at which withdrawal by users is enabled.

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L39
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L53

With each call activated[_token] is reset to block.timestamp. Thus the require statement in withdraw() will fail for another week. There's nothing stopping the client from calling the function multiple times to keep delaying the withdrawal.

Fix it by not allowing already activated tokens to be reactivated. But with the current implementation of the contract, the client could deactivate and reactivate it.

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

Report

• cache values from external function calls
• remove unnecessary safeApprove()
• use constant state variables wherever you can
• use immutable state variables wherever you can

cache values from external function calls

Instead of calling the same function twice, cache the return value and use that.

Example:

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L131

Replace the call with extraToken which was cached here: https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L123

remove unnecessary safeApprove()

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L242-L244

The second safeApprove() call can be removed. After the deposit() the convexBooster's allowance should be 0 already.

use constant state variables wherever you can

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L56-L57
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L50

use immutable state variables wherever you can

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol#L19-L20
• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L22-L24