Concur Finance contest - Dravee's results

Lines of code

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

Vulnerability details

Impact

Wrong internal accounting

Proof of Concept

In this external function, an arbitrary ERC20 token can be passed as an argument:

File: Shelter.sol
32:     function donate(IERC20 _token, uint256 _amount) external {
33:         require(activated[_token] != 0, "!activated");
34:         savedTokens[_token] += _amount; //@audit missing FOT checks...
35:         _token.safeTransferFrom(msg.sender, address(this), _amount); //@audit missing FOT checks...
36:     }

The received amount should be calculated every time to take into consideration a possible transfer-on-fee or deflation.

Tools Used

VS Code

Recommended Mitigation Steps

Check the balance before and after the transfer to take into account the Fees-On-Transfer. As an example:

File: Shelter.sol
32:     function donate(IERC20 _token, uint256 _amount) external {
33:         require(activated[_token] != 0, "!activated");
34:         uint256 balanceBefore = _token.balanceOf(address(this));
35:         _token.safeTransferFrom(msg.sender, address(this), _amount);
36:         uint256 receivedAmount = _token.balanceOf(address(this)) - balanceBefore;
37:         savedTokens[_token] += receivedAmount; 
38:         //@audit add an emit?
39:     }

Emitting an event should also be considered.

QA Report

Table of Contents:

• QA Report
  • Transfers
    • Prevent accidentally burning tokens
    • Use safeTransfer or require()/conditional instead of transfer/transferFrom
    • Use SafeERC20.safeApprove()
  • Libraries
    • Deprecated library used for Solidity 0.8.11: SafeMath
  • Variables
    • Missing Address(0) checks
    • Variables that should be constant
    • Variables that are assumed to be initialized before a function call, but might not be
    • Variables that should be bounded
    • Variables that should be grouped together in a struct
      • File: ConvexStakingWrapper.sol
      • File: Shelter.sol
  • Functions
    • Functions that should be declared external
  • Arithmetics
    • Possible division by 0

Transfers

Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Places where I couldn't find a zero address check (or where the destination isn't a zero-checked address):

ConvexStakingWrapper.sol:179:            IERC20(reward.token).transfer(treasury, d_reward / 5); //@audit treasury isn't address(0) checked
ConvexStakingWrapper.sol:182:        IERC20(reward.token).transfer(address(claimContract), d_reward);//@audit claimContract isn't address(0) checked
MasterChef.sol:206:            transferSuccess = concur.transfer(_to, concurBalance);//@audit _to == _recipient and isn't address(0) checked
MasterChef.sol:208:            transferSuccess = concur.transfer(_to, _amount); //@audit _to == _recipient and isn't address(0) checked

I suggest adding a check to prevent accidentally burning tokens.

Use safeTransfer or require()/conditional instead of transfer/transferFrom

Silent failures (lack of failure detection / revert in case of failure) may happen here:

File: ConvexStakingWrapper.sol
178:         if (reward.token == cvx || reward.token == crv) {
179:             IERC20(reward.token).transfer(treasury, d_reward / 5); //@audit return value ignored
180:             d_reward = (d_reward * 4) / 5;
181:         }
182:         IERC20(reward.token).transfer(address(claimContract), d_reward);//@audit return value ignored

Consider using safeTransfer. That's already the case at other places on the same contract

Use SafeERC20.safeApprove()

approve() will fail for certain token implementations that do not return a boolean value. It is recommended to use OpenZeppelin's SafeERC20's safeApprove().

Instances include:

USDMPegRecovery.sol:79:        usdm.approve(address(usdm3crv), addingLiquidity);
USDMPegRecovery.sol:80:        pool3.approve(address(usdm3crv), addingLiquidity);

Libraries

Deprecated library used for Solidity 0.8.11: SafeMath

Use Solidity 0.8.*'s default checks instead:

MasterChef.sol:10:import "@openzeppelin/contracts/utils/math/SafeMath.sol";
MasterChef.sol:14:    using SafeMath for uint;

Variables

Missing Address(0) checks

		- rewardNotifier = _notifier (contracts/ConcurRewardPool.sol#16)
		- treasury = _treasury (contracts/ConvexStakingWrapper.sol#70)
		- treasury = _treasury (contracts/ConvexStakingWrapper.sol#83)
		- rewardsDistribution = _rewardsDistribution (contracts/StakingRewards.sol#45)
		- rewardsDistribution = _rewardsDistribution (contracts/StakingRewards.sol#195)
		- kpiOracle = _kpiOracle (contracts/USDMPegRecovery.sol#57)
		- (success,result) = _to.call{value: _value}(_data) (contracts/VoteProxy.sol#33)

Variables that should be constant

MasterChef._concurShareMultiplier (contracts/MasterChef.sol#56)
MasterChef._perMille (contracts/MasterChef.sol#57)
MasterChef.concurPerBlock (contracts/MasterChef.sol#50)

Variables that are assumed to be initialized before a function call, but might not be

File: ConvexStakingWrapper.sol
50:     IConcurRewardClaim public claimContract;
...
86:     function setRewardPool(address _claimContract) external onlyOwner {
87:         claimContract = IConcurRewardClaim(_claimContract);
88:     }

Variables that should be bounded

The variable MasterChef.sol:43: uint16 depositFeeBP; // Deposit fee in basis points is never bounded, and UInt16.MaxValue is 65535

Variables that should be grouped together in a struct

For maps that use the same key value: having separate fields is error prone (like in case of deletion or future new fields).

File: ConvexStakingWrapper.sol

6 maps can be grouped together, as they use the same pid:

41:     //convex rewards
42:     mapping(uint256 => address) public convexPool;
43:     mapping(uint256 => RewardType[]) public rewards;
44:     mapping(uint256 => mapping(uint256 => mapping(address => Reward)))
45:         public userReward;
46:     mapping(uint256 => mapping(address => uint256)) public registeredRewards;
...
63:     mapping(uint256 => mapping(address => Deposit)) public deposits;
64:     mapping(uint256 => mapping(address => WithdrawRequest)) public withdrawRequest;

I'd suggest these 3 related data get grouped in a struct, let's name it RewardInfo:

struct RewardInfo {  
  address convexPool;  
  RewardType[] rewards;  
  mapping(uint256 => mapping(address => Reward)) userReward;  
  mapping(address => uint256) registeredRewards;  
  mapping(address => Deposit) deposits;  
  mapping(address => WithdrawRequest) withdrawRequest;  
}  

And it would be used as a state variable in this manner (where uint256 is pid):

mapping(uint256 => RewardInfo) rewardInfo;  

File: Shelter.sol

3 maps can be grouped together, as they use the same _token:

17:     mapping(IERC20 => mapping(address => bool)) public override claimed;
18: 
19:     mapping(IERC20 => uint256) public activated;
20: 
21:     mapping(IERC20 => uint256) public savedTokens;

I'd suggest these 3 related data get grouped in a struct, let's name it TokenInfo:

struct TokenInfo {  
  mapping(address => bool) claimed;  
  uint256 activated;  
  uint256 savedTokens;  
}  

And it would be used as a state variable in this manner (where IERC20 is _token):

mapping(IERC20 => TokenInfo) tokenInfo;  

Functions

Functions that should be declared external

	- ConvexStakingWrapper.addRewards(uint256) (contracts/ConvexStakingWrapper.sol#93-140)
	- MasterChef.add(address,uint256,uint16,uint256) (contracts/MasterChef.sol#86-101)
	- MasterChef.massUpdatePools() (contracts/MasterChef.sol#127-132)

Arithmetics

Possible division by 0

There are no checks that the denominator is != 0 at thoses lines:

library\CvxMining.sol:16:        uint256 cliff = supply / reductionPerCliff;
MasterChef.sol:120:            uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
MasterChef.sol:121:            accConcurPerShare = accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
MasterChef.sol:151:        uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
MasterChef.sol:152:        pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
Shelter.sol:54:        uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token);

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • File: ConcurRewardPool.sol
    • function claimRewards()
      • An array's length should be cached to save gas in for-loops
      • A variable used only once and shouldn't get cached
  • File: ConvexStakingWrapper.sol
    • Storage
      • Struct with only 1 element
    • function withdraw()
      • Double external call: create a function in the external contract
  • File: MasterChef.sol
    • Contract declaration
      • SafeMath is not needed when using Solidity version 0.8.11
      • Tightly pack struct PoolInfo
    • function add()
      • The require statement can be placed earlier to reduce gas usage on revert.
      • Unchecked block
    • function pendingConcur()
      • Cache pool.lastRewardBlock and _concurShareMultiplier
    • function updatePool
      • Cache pool.lastRewardBlock and pool.allocPoint
    • function deposit()
      • Cache user.amount
      • Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used
    • function withdraw()
      • Cache user.amount
      • Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used
      • Unchecked block
  • File: EasySign.sol
    • function isWinningSignature()
      • Use calldata instead of memory for external functions where the function argument is read-only.
      • A variable used only once and shouldn't get cached
  • File: VoteProxy.sol
    • function isValidSignature()
      • Boolean comparisons
  • File: USDMPegRecovery.sol
    • function provide()
      • Cache usdm.balanceOf(address(this)), step and address(usdm3crv)
  • File: StakingRewards.sol
    • function lastTimeRewardApplicable()
      • Cache periodFinish
    • function rewardPerToken()
      • Cache _totalSupply
    • function withdraw()
      • A variable used only once and shouldn't get cached
    • function notifyRewardAmount()
      • Cache periodFinish, rewardsDuration and rewardRate
      • Unchecked block
    • function recoverERC20()
      • Cache periodFinish, rewardsDuration and rewardRate
      • Unchecked block
      • Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used
    • function setRewardsDuration()
      • Emitting a storage value
    • function setRewardsDistribution()
      • Emitting a storage value
  • File: Shelter.sol
    • Contract declaration
      • Unnecessary "override" keyword
    • function deactivate()
      • Cache activated[_token]
    • function withdraw()
      • Cache activated[_token] and client
  • File: CvxMining.sol
    • function ConvertCrvToCvx()
      • Unchecked block
  • General recommendations
    • Variables
      • No need to explicitly initialize variables with default values
    • Arithmetics
      • Unchecking arithmetics operations that can't underflow/overflow
    • Comparisons
      • > 0 is less efficient than != 0 for unsigned integers (with proof)
      • >= is cheaper than >
    • Amounts should be checked for 0 before calling a transfer
    • For-Loops
      • ++i costs less gas compared to i++
      • Increments can be unchecked
    • Visibility
      • Consider making some constants as non-public to save gas
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

• Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

File: ConcurRewardPool.sol

function claimRewards()

File: ConcurRewardPool.sol
34:     function claimRewards(address[] calldata _tokens) external override {
35:         for (uint256 i = 0; i < _tokens.length; i++) { //@audit cache "_tokens.length" in memory
36:             uint256 getting = reward[msg.sender][_tokens[i]]; //@audit "getting" used only once
37:             IERC20(_tokens[i]).safeTransfer(msg.sender, getting);
38:             reward[msg.sender][_tokens[i]] = 0;
39:         }
40:     }

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

I suggest storing the array's length in a variable before the for-loop, and use it instead.

A variable used only once and shouldn't get cached

I suggest you inline the getting variable, which is used only once, to save a MSTORE + a MLOAD (6 gas) per iteration

File: ConvexStakingWrapper.sol

Storage

Struct with only 1 element

It is not efficient to have a struct with only 1 field as structs are meant for grouping related information together. A Reward struct can be replaced by directly pointing to a uint128 value.

I suggest changing the code from this:

File: ConvexStakingWrapper.sol
24:     struct Reward {
25:         uint128 integral; 
26:     }
...
43:     mapping(uint256 => RewardType[]) public rewards;
44:     mapping(uint256 => mapping(uint256 => mapping(address => Reward)))
45:         public userReward;

to this:

43:     mapping(uint256 => uint128[]) public rewards;
44:     mapping(uint256 => mapping(uint256 => mapping(address => uint128)))
45:         public userReward;

function withdraw()

File: ConvexStakingWrapper.sol
263:             IRewardStaking(convexPool[_pid]).withdrawAndUnwrap(_amount, false); //@audit 1st call to IRewardStaking(convexPool[_pid])
264:             IERC20 lpToken = IERC20(
265:                 IRewardStaking(convexPool[_pid]).poolInfo(_pid).lptoken //@audit 2nd call to IRewardStaking(convexPool[_pid])
266:             );

Double external call: create a function in the external contract

As there are 2 consecutive calls to IRewardStaking(convexPool[_pid]), I suggest adding a new method in the external contract that would do both and return a IERC20 lpToken.

File: MasterChef.sol

Contract declaration

SafeMath is not needed when using Solidity version 0.8.11

Solidity version 0.8.11 already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8.* overflow checks) is therefore redundant.

I suggest using the built-in checks instead of SafeMath and remove SafeMath here:

MasterChef.sol:10:import "@openzeppelin/contracts/utils/math/SafeMath.sol";
MasterChef.sol:14:    using SafeMath for uint;

Tightly pack struct PoolInfo

struct PoolInfo can be tightly packed to save 1 storage slot by changing the code from this:

    struct PoolInfo { 
        IERC20 depositToken; // 20 bytes
        uint allocPoint; // 32 bytes
        uint lastRewardBlock; // 32 bytes
        uint accConcurPerShare; // 32 bytes
        uint16 depositFeeBP; // 2 bytes
    }

to this:

    struct PoolInfo { 
        IERC20 depositToken; // 20 bytes
        uint16 depositFeeBP; // 2 bytes
        uint allocPoint; // 32 bytes
        uint lastRewardBlock; // 32 bytes
        uint accConcurPerShare; // 32 bytes
    }

function add()

File: MasterChef.sol
086:     function add(address _token, uint _allocationPoints, uint16 _depositFee, uint _startBlock) public onlyOwner {
087:         require(_token != address(0), "zero address");
088:         uint lastRewardBlock = block.number > _startBlock ? block.number : _startBlock;
089:         totalAllocPoint = totalAllocPoint.add(_allocationPoints);
090:         require(pid[_token] == 0, "already registered"); // pid starts from 0 //@audit can be moved above to save gas on revert
091:         poolInfo.push(
092:             PoolInfo({
093:                 depositToken: IERC20(_token),
094:                 allocPoint: _allocationPoints,
095:                 lastRewardBlock: lastRewardBlock,
096:                 accConcurPerShare: 0,
097:                 depositFeeBP: _depositFee
098:             })
099:         );
100:         pid[_token] = poolInfo.length - 1; //@audit can be unchecked as the push L91 makes it >= 1
101:     }

The require statement can be placed earlier to reduce gas usage on revert.

The require statement line 90 can be moved just after the first require statement line 87 to reduce gas usage on revert, saving notably a SSTORE and a first SLOAD in case of revert.

Unchecked block

As poolInfo.length has a minimum length of 1 due to the push at line 91, it's impossible for line 100 to underflow. Therefore, it should be wrapped inside an unchecked block.

function pendingConcur()

Cache pool.lastRewardBlock and _concurShareMultiplier

File: MasterChef.sol
118:         if (block.number > pool.lastRewardBlock && lpSupply != 0) { //@audit pool.lastRewardBlock SLOAD 1
119:             uint multiplier = getMultiplier(pool.lastRewardBlock, block.number); //@audit pool.lastRewardBlock SLOAD 2
120:             uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
121:             accConcurPerShare = accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply)); //@audit _concurShareMultiplier SLOAD 1
122:         }
123:         return user.amount * accConcurPerShare / _concurShareMultiplier - user.rewardDebt; //@audit _concurShareMultiplier SLOAD 2

Caching these in memory can save around 2 SLOADs

function updatePool

Cache pool.lastRewardBlock and pool.allocPoint

135:     function updatePool(uint _pid) public {
136:         PoolInfo storage pool = poolInfo[_pid];
137:         if (block.number <= pool.lastRewardBlock) {//@audit pool.lastRewardBlock SLOAD 1
138:             return;
139:         }
140:         uint lpSupply = pool.depositToken.balanceOf(address(this));
141:         if (lpSupply == 0 || pool.allocPoint == 0) { //@audit pool.allocPoint SLOAD 1
142:             pool.lastRewardBlock = block.number;
143:             return;
144:         }
145:         if(block.number >= endBlock) {
146:             pool.lastRewardBlock = block.number;
147:             return;
148:         }        
149: 
150:         uint multiplier = getMultiplier(pool.lastRewardBlock, block.number);  //@audit pool.lastRewardBlock SLOAD 2
151:         uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint); //@audit pool.allocPoint SLOAD 2
152:         pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
153:         pool.lastRewardBlock = block.number;
154:     }

Caching these in memory can save around 2 SLOADs

function deposit()

File: MasterChef.sol
157:     function deposit(address _recipient, uint _pid, uint _amount) external nonReentrant onlyDepositor {
158:         PoolInfo storage pool = poolInfo[_pid];
159:         UserInfo storage user = userInfo[_pid][_msgSender()]; //@audit msg.sender 
160:         updatePool(_pid);
161:        
162:         if(user.amount > 0) {  //@audit user.amount SLOAD 1
163:             uint pending = user.amount * pool.accConcurPerShare / _concurShareMultiplier - user.rewardDebt; //@audit user.amount SLOAD 2
164:             if (pending > 0) {
165:                 safeConcurTransfer(_recipient, pending);
166:             }
167:         }
168: 
169:         if (_amount > 0) {
170:             if (pool.depositFeeBP > 0) {
171:                 uint depositFee = _amount.mul(pool.depositFeeBP).div(_perMille);
172:                 user.amount = SafeCast.toUint128(user.amount + _amount - depositFee);//@audit user.amount SLOAD 3
173:             } else {
174:                 user.amount = SafeCast.toUint128(user.amount + _amount); //@audit user.amount SLOAD 3
175:             }
176:         }     
177: 
178:         user.rewardDebt = SafeCast.toUint128(user.amount * pool.accConcurPerShare / _concurShareMultiplier); //@audit user.amount SLOAD 4
179:         emit Deposit(_recipient, _pid, _amount);
180:     }

Cache user.amount

Caching in memory can save around 3 SLOADs. I suggest using memory variables for all calculation (line 178 can use a re-calculated & recached userAmount if lines 172 or 174 are executed. It's not necessary to re-read storage)

Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used

msg.sender costs 2 gas (CALLER opcode). The default _msgSender() represents the following:

function _msgSender() internal view virtual returns (address payable) {
  return msg.sender;
}

When no GSN capabilities are used: msg.sender is enough.

I suggest replacing _msgSender() with msg.sender

See https://docs.openzeppelin.com/contracts/2.x/gsn for more information about GSN capabilities.

function withdraw()

File: MasterChef.sol
183:     function withdraw(address _recipient, uint _pid, uint _amount) external nonReentrant onlyDepositor {
184:         PoolInfo storage pool = poolInfo[_pid];
185:         UserInfo storage user = userInfo[_pid][_msgSender()]; //@audit msg.sender
186:         require(user.amount > 0, "MasterChef: nothing to withdraw"); //@audit user.amount SLOAD 1
187:         require(user.amount >= _amount, "MasterChef: withdraw not allowed");//@audit user.amount SLOAD 2
188:         updatePool(_pid);
189: 
190:         uint pending = user.amount * pool.accConcurPerShare / _concurShareMultiplier - user.rewardDebt; //@audit user.amount SLOAD 3
191:         if(pending > 0) {
192:             safeConcurTransfer(_recipient, pending);
193:         }
194:         if (_amount > 0) {
195:             user.amount = SafeCast.toUint128(user.amount - _amount); //@audit can be unchecked due to line 187 //@audit user.amount SLOAD 4
196:         }
197:         user.rewardDebt = SafeCast.toUint128(user.amount * pool.accConcurPerShare / _concurShareMultiplier); //@audit user.amount SLOAD 5 (even if changed line 195, the calculation result can use memory and be cached and used here)
198:         emit Withdraw(_recipient, _pid, _amount);
199:     }

Cache user.amount

Caching in memory can save around 4 SLOADs.

Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used

msg.sender costs 2 gas (CALLER opcode). The default _msgSender() represents the following:

function _msgSender() internal view virtual returns (address payable) {
  return msg.sender;
}

When no GSN capabilities are used: msg.sender is enough.

Unchecked block

user.amount - _amount at line 195 can't underflow due to require(user.amount >= _amount, "MasterChef: withdraw not allowed") line 187. Therefore, line 195 should be wrapped inside an unchecked block.

File: EasySign.sol

function isWinningSignature()

File: EasySign.sol
214:     function isWinningSignature(bytes32 _hash, bytes memory _signature) //@audit _signature is read-only
215:         external
216:         view
217:         override
218:         returns (bool)
219:     {
220:         address signer = recover(_hash, _signature); //@audit only _signature use
221:         return approvedTeam[signer];
222:     }

Use calldata instead of memory for external functions where the function argument is read-only.

On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory :

memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Here, I suggest using calldata instead of memory as, in this external function, the memory argument is read-only.

A variable used only once and shouldn't get cached

I suggest you inline the signer variable, which is used only once, to save a MSTORE + a MLOAD (6 gas)

File: VoteProxy.sol

function isValidSignature()

File: VoteProxy.sol
15:     function isValidSignature(bytes32 _hash, bytes calldata _signature)
16:         external
17:         view
18:         returns (bytes4)
19:     {
20:         // Validate signatures
21:         if (auctioneer.isWinningSignature(_hash, _signature) == true) { //@audit "== true" not necessary
22:             return 0x1626ba7e;
23:         } else {
24:             return 0xffffffff;
25:         }
26:     }

Boolean comparisons

At line 21, if (auctioneer.isWinningSignature(_hash, _signature)) would be a bit cheaper than the comparison to a constant.

File: USDMPegRecovery.sol

function provide()

File: USDMPegRecovery.sol
73:     function provide(uint256 _minimumLP) external onlyGuardian {
74:         require(usdm.balanceOf(address(this)) >= totalLiquidity.usdm, "<liquidity");//@audit usdm.balanceOf(address(this)) SLOAD 1
75:         // truncate amounts under step
76:         uint256 addingLiquidity = (usdm.balanceOf(address(this)) / step) * step;//@audit usdm.balanceOf(address(this)) SLOAD 2 //@audit step SLOAD 1 & 2
77:         // match usdm : pool3 = 1 : 1
78:         uint256[2] memory amounts = [addingLiquidity, addingLiquidity];
79:         usdm.approve(address(usdm3crv), addingLiquidity); //@audit usdm3crv SLOAD 1
80:         pool3.approve(address(usdm3crv), addingLiquidity); //@audit usdm3crv SLOAD 2
81:         usdm3crv.add_liquidity(amounts, _minimumLP);
82:     }

Cache usdm.balanceOf(address(this)), step and address(usdm3crv)

Caching these in memory can save around 3 SLOADs

File: StakingRewards.sol

function lastTimeRewardApplicable()

File: StakingRewards.sol
59:     function lastTimeRewardApplicable() public view returns (uint256) {
60:         return block.timestamp < periodFinish ? block.timestamp : periodFinish; //@audit periodFinish SLOAD 1 & 2
61:     }

Cache periodFinish

Caching this in memory can save around 1 SLOAD

function rewardPerToken()

File: StakingRewards.sol
63:     function rewardPerToken() public view returns (uint256) {
64:         if (_totalSupply == 0) { //@audit _totalSupply SLOAD 1
65:             return rewardPerTokenStored;
66:         }
67:         return
68:             rewardPerTokenStored +
69:             (((lastTimeRewardApplicable() - lastUpdateTime) *
70:                 rewardRate *
71:                 1e18) / _totalSupply); //@audit _totalSupply SLOAD 2
72:     }

Cache _totalSupply

Caching this in memory can save around 1 SLOAD

function withdraw()

File: StakingRewards.sol
103:     function withdraw(uint256 amount)
104:         public
105:         nonReentrant
106:         updateReward(msg.sender)
107:     {
108:         require(amount > 0, "Cannot withdraw 0");
109:         _totalSupply -= amount;
110:         _balances[msg.sender] -= amount;
111:         stakingToken.safeTransfer(msg.sender, amount);
112:         uint256 pid = masterChef.pid(address(stakingToken)); //@audit pid used only once
113:         masterChef.withdraw(msg.sender, pid, amount);
114:         emit Withdrawn(msg.sender, amount);
115:     }

A variable used only once and shouldn't get cached

I suggest you inline the pid variable, which is used only once, to save a MSTORE + a MLOAD (6 gas) per iteration

function notifyRewardAmount()

133:     function notifyRewardAmount(uint256 reward)
134:         external
135:         updateReward(address(0))
136:     {
137:         require(
138:             msg.sender == rewardsDistribution,
139:             "Caller is not RewardsDistribution contract"
140:         );
141: 
142:         if (block.timestamp >= periodFinish) { //@audit periodFinish SLOAD 1
143:             rewardRate = reward / rewardsDuration; //@audit rewardsDuration SLOAD 1
144:         } else {
145:             uint256 remaining = periodFinish - block.timestamp; //@audit can be unchecked due to else statement (periodFinish > block.timestamp) //@audit periodFinish SLOAD 2
146:             uint256 leftover = remaining * rewardRate; //@audit rewardRate SLOAD 1
147:             rewardRate = (reward + leftover) / rewardsDuration; //@audit rewardsDuration SLOAD 1
148:         }
149: 
150:         // Ensure the provided reward amount is not more than the balance in the contract.
151:         // This keeps the reward rate in the right range, preventing overflows due to
152:         // very high values of rewardRate in the earned and rewardsPerToken functions;
153:         // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
154:         uint256 balance = rewardsToken.balanceOf(address(this));
155:         require(
156:             rewardRate <= balance / rewardsDuration, //@audit rewardRate SLOAD 2 //@audit rewardsDuration SLOAD 2
157:             "Provided reward too high"
158:         );
159: 
160:         lastUpdateTime = block.timestamp;
161:         periodFinish = block.timestamp + rewardsDuration; //@audit rewardsDuration SLOAD 3
162:         emit RewardAdded(reward);
163:     }

Cache periodFinish, rewardsDuration and rewardRate

Caching these in memory and using them for calculation can save around 4 SLOADs

Unchecked block

periodFinish - block.timestamp at line 145 can't underflow due to if (block.timestamp >= periodFinish) line 142. Therefore, line 145 should be wrapped inside an unchecked block.

function recoverERC20()

File: StakingRewards.sol
166:     function recoverERC20(address tokenAddress, uint256 tokenAmount) //@audit isn't it sensitive enough to need a timelock? (Med risk)
167:         external
168:         onlyOwner
169:     {
170:         require(
171:             tokenAddress != address(stakingToken),
172:             "Cannot withdraw the staking token"
173:         );
174:         IERC20(tokenAddress).safeTransfer(owner(), tokenAmount); //@audit gas: msg.sender is enough
175:         emit Recovered(tokenAddress, tokenAmount);
176:     }

Cache periodFinish, rewardsDuration and rewardRate

Caching these in memory and using them for calculation can save around 4 SLOADs

Unchecked block

periodFinish - block.timestamp at line 145 can't underflow due to if (block.timestamp >= periodFinish) line 142. Therefore, line 145 should be wrapped inside an unchecked block.

Use msg.sender instead of OpenZeppelin's _msgSender() when GSN capabilities aren't used

msg.sender costs 2 gas (CALLER opcode). The default _msgSender() represents the following:

function _msgSender() internal view virtual returns (address payable) {
  return msg.sender;
}

When no GSN capabilities are used: msg.sender is enough.

function setRewardsDuration()

File: StakingRewards.sol
178:     function setRewardsDuration(uint256 _rewardsDuration) external onlyOwner {
179:         require(
180:             block.timestamp > periodFinish,
181:             "Previous rewards period must be complete before changing the duration for the new period"
182:         );
183:         rewardsDuration = _rewardsDuration;
184:         emit RewardsDurationUpdated(rewardsDuration); //@audit emitting a storage value
185:     }

Emitting a storage value

Line 184 should be replaced with emit RewardsDurationUpdated(_rewardsDuration);

function setRewardsDistribution()

File: StakingRewards.sol
187:     function setRewardsDistribution(address _rewardsDistribution)
188:         external
189:         onlyOwner
190:     {
191:         require(
192:             block.timestamp > periodFinish,
193:             "Previous rewards period must be complete before changing the duration for the new period"
194:         );
195:         rewardsDistribution = _rewardsDistribution;
196:         emit RewardsDistributionUpdated(rewardsDistribution); //@audit emitting a storage value
197:     }

Emitting a storage value

Line 196 should be replaced with emit RewardsDistributionUpdated(_rewardsDuration);

File: Shelter.sol

Contract declaration

File: Shelter.sol
17:     mapping(IERC20 => mapping(address => bool)) public override claimed; //@audit override?

Unnecessary "override" keyword

The override keyword isn't necessary at line 17.

function deactivate()

File: Shelter.sol
44:     function deactivate(IERC20 _token) external override onlyClient {
45:         require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD > block.timestamp, "too late"); //@audit activated[_token] SLOAD 1 & 2
46:         activated[_token] = 0;
47:         savedTokens[_token] = 0;
48:         _token.safeTransfer(msg.sender, _token.balanceOf(address(this)));
49:         emit ShelterDeactivated(_token);
50:     }

Cache activated[_token]

Caching this in memory can save around 1 SLOAD

function withdraw()

File: Shelter.sol
52:     function withdraw(IERC20 _token, address _to) external override {
53:         require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated"); //@audit activated[_token] SLOAD 1 & 2
54:         uint256 amount = savedTokens[_token] * client.shareOf(_token, msg.sender) / client.totalShare(_token); //@audit client SLOAD 1 & 2
55:         claimed[_token][_to] = true;
56:         emit ExitShelter(_token, msg.sender, _to, amount);
57:         _token.safeTransfer(_to, amount);
58:     }

Cache activated[_token] and client

Caching these in memory can save around 2 SLOADs

File: CvxMining.sol

function ConvertCrvToCvx()

18:         if(cliff < totalCliffs){
19:             //for reduction% take inverse of current cliff
20:             uint256 reduction = totalCliffs - cliff; //@audit this line can be unchecked due to line 18
21:             //reduce

Unchecked block

totalCliffs - cliff at line 20 can't underflow due to if(cliff < totalCliffs) line 18. Therefore, line 20 should be wrapped inside an unchecked block.

General recommendations

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

ConcurRewardPool.sol:35:        for (uint256 i = 0; i < _tokens.length; i++) {
ConvexStakingWrapper.sol:36:    uint256 public constant CRV_INDEX = 0;
ConvexStakingWrapper.sol:121:        for (uint256 i = 0; i < extraCount; i++) {
ConvexStakingWrapper.sol:219:        for (uint256 i = 0; i < rewardCount; i++) {
MasterChef.sol:51:    uint public totalAllocPoint = 0; // Total allocation points. Must be the sum of all allocation points in all pools.
MasterChef.sol:129:        for (uint _pid = 0; _pid < length; ++_pid) {
MasterChef.sol:204:        bool transferSuccess = false;
StakingRewards.sol:21:    uint256 public periodFinish = 0;
StakingRewards.sol:22:    uint256 public rewardRate = 0;

I suggest removing explicit initializations for default values.

Arithmetics

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Comparisons

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

> 0 in require statements are used in the following location(s):

MasterChef.sol:186:        require(user.amount > 0, "MasterChef: nothing to withdraw");
StakingRewards.sol:94:        require(amount > 0, "Cannot stake 0");
StakingRewards.sol:108:        require(amount > 0, "Cannot withdraw 0");

I suggest you change > 0 with != 0 in require statements. Also, enable the Optimizer.

>= is cheaper than >

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

I suggest using >= instead of > to avoid some opcodes at those places:

MasterChef.sol:88:        uint lastRewardBlock = block.number > _startBlock ? block.number : _startBlock;
MasterChef.sol:205:        if (_amount > concurBalance) {
Shelter.sol:45:        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD > block.timestamp, "too late");
Shelter.sol:53:        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
StakingRewards.sol:60:        return block.timestamp < periodFinish ? block.timestamp : periodFinish;

Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas.

Places I suggest adding a non-zero-value check:

ConcurRewardPool.sol:37:            IERC20(_tokens[i]).safeTransfer(msg.sender, getting); //@audit calling safeTransfer in a for-loop without checking for 0 amount
ConvexStakingWrapper.sol:179:            IERC20(reward.token).transfer(treasury, d_reward / 5); //@audit 0 amount can happen if balance == remaining rewards
ConvexStakingWrapper.sol:182:        IERC20(reward.token).transfer(address(claimContract), d_reward);//@audit 0 amount can happen if balance == remaining rewards
Shelter.sol:57:        _token.safeTransfer(_to, amount); //@audit unlikely to be 0 so could maybe be ignored
StakingRewards.sol:174:        IERC20(tokenAddress).safeTransfer(owner(), tokenAmount); //@audit unlikely to be 0 (external function argument)

For-Loops

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

ConcurRewardPool.sol:35:        for (uint256 i = 0; i < _tokens.length; i++) {
ConvexStakingWrapper.sol:121:        for (uint256 i = 0; i < extraCount; i++) {
ConvexStakingWrapper.sol:219:        for (uint256 i = 0; i < rewardCount; i++) {
MasterChef.sol:129:        for (uint _pid = 0; _pid < length; ++_pid) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

ConcurRewardPool.sol:35:        for (uint256 i = 0; i < _tokens.length; i++) {
ConvexStakingWrapper.sol:121:        for (uint256 i = 0; i < extraCount; i++) {
ConvexStakingWrapper.sol:219:        for (uint256 i = 0; i < rewardCount; i++) {
MasterChef.sol:129:        for (uint _pid = 0; _pid < length; ++_pid) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}  

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

The risk of overflow is inexistant for a uint256 here.

Visibility

Consider making some constants as non-public to save gas

Reducing from public to private or internal can save gas. All these constants aren't used outside of their contract:

library\CvxMining.sol:8:    ICvx public constant cvx = ICvx(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B);
ConvexStakingWrapper.sol:29:    address public constant convexBooster =
ConvexStakingWrapper.sol:31:    address public constant crv =
ConvexStakingWrapper.sol:33:    address public constant cvx =
ConvexStakingWrapper.sol:36:    uint256 public constant CRV_INDEX = 0;
ConvexStakingWrapper.sol:37:    uint256 public constant CVX_INDEX = 1;
ConvexStakingWrapper.sol:38:    uint256 public constant VOTECYCLE_START = 1645002000; //Feb 16 2022 09:00:00 GMT+0000
Shelter.sol:15:    uint256 public constant GRACE_PERIOD = 1 weeks;

I suggest changing their visibility from public to internal or private

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes are here:

EasySign.sol:25:            revert("ECDSA: invalid signature 's' value");
EasySign.sol:27:            revert("ECDSA: invalid signature 'v' value");
MasterChef.sol:210:        require(transferSuccess, "safeConcurTransfer: transfer failed");
StakingRewards.sol:139:            "Caller is not RewardsDistribution contract"
StakingRewards.sol:172:            "Cannot withdraw the staking token"
StakingRewards.sol:181:            "Previous rewards period must be complete before changing the duration for the new period"
StakingRewards.sol:193:            "Previous rewards period must be complete before changing the duration for the new period" 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

ConcurRewardPool.sol:28:        require(msg.sender == rewardNotifier, "!notifier");
ConvexStakingWrapper.sol:258:        require(request.epoch < currentEpoch() && deposits[_pid][msg.sender].epoch + 1 < currentEpoch(), "wait");
ConvexStakingWrapper.sol:259:        require(request.amount >= _amount, "too much");
ConvexStakingWrapper.sol:287:        require(_amount <= uint256(deposits[_pid][msg.sender].amount), "too much");
MasterChef.sol:74:        require(isDepositor[msg.sender], "!depositor");
MasterChef.sol:87:        require(_token != address(0), "zero address");
MasterChef.sol:90:        require(pid[_token] == 0, "already registered"); // pid starts from 0
MasterChef.sol:186:        require(user.amount > 0, "MasterChef: nothing to withdraw");
MasterChef.sol:187:        require(user.amount >= _amount, "MasterChef: withdraw not allowed");
MasterChef.sol:210:        require(transferSuccess, "safeConcurTransfer: transfer failed");
Shelter.sol:24:        require(msg.sender == address(client), "!client");
Shelter.sol:33:        require(activated[_token] != 0, "!activated");
Shelter.sol:45:        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD > block.timestamp, "too late");
Shelter.sol:53:        require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD < block.timestamp, "shelter not activated");
StakingRewards.sol:94:        require(amount > 0, "Cannot stake 0");
StakingRewards.sol:108:        require(amount > 0, "Cannot withdraw 0");
StakingRewards.sol:137:        require(
StakingRewards.sol:155:        require(
StakingRewards.sol:170:        require(
StakingRewards.sol:179:        require(
StakingRewards.sol:191:        require(
USDMPegRecovery.sol:44:        require(isGuardian[msg.sender], "!guardian");
USDMPegRecovery.sol:69:        require(msg.sender == kpiOracle, "!oracle");
USDMPegRecovery.sol:74:        require(usdm.balanceOf(address(this)) >= totalLiquidity.usdm, "<liquidity");
USDMPegRecovery.sol:100:            require(totalLiquidity.usdm > 4000000e18, "usdm low");
USDMPegRecovery.sol:114:            require(unlockable, "!unlock usdm");

I suggest replacing revert strings with custom errors.