Concur Finance contest - SolidityScan's results

Lines of code

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

Vulnerability details

Description

The contract was found vulnerable to Reentrancy attack. It was noticed that the function claimRewards makes an external call to another untrusted address or a contract before it resolves any effects and unlike other functions, it is lacking nonReentrant modifier.

If the attacker controls the untrusted contract, they may be able to call back to the original function, repeating interactions that would have otherwise not run after the effects were resolved.

Impact

Reentrancy vulnerabilities can lead to various critical outcomes such as token stealing and burning. Adversaries may exploit the bug to mint tokens without any limitations or extract all the tokens out of the contract.

• Here this vulnerability may lead to the malicious user extracting more rewards or tokens from the contract than the actual amount stored for them.

PoC

• Go to https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L34-L40
• We will notice the function claimRewards is performing an external call at the line https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L37 but is missing the nonReentrant modifier during the function call.

Recommended Mitigation Steps

Introduce a modifier nonreentrant to prevent Reentrancy vulnerabilities by implementing a Check-Effects-Interactions pattern.

Summary:

During the code assessment, we found multiple issues related to unchecked transfer and no handing of return values. Many functions calls like transfer, deposit, approve, etc., return some value after the function call. Handling these calls is important to prevent unexpected outcomes. Apart from that, we found multiple functions which were missing zero address checks. It is advised to add a zero address check at all possible functions setting an address. In solidity, any error caused to set the value to default, and for an address variable, the default value is zero address. Hence any fund or privilege gets pointed to zero address, and it will be unrecoverable. During the code review, we noticed boolean comparison, which is redundant as it is auto compared when an if statement is used; hence it can be removed. We also noticed missing events in many critical functions. Events are important for logging purposes. Hence it is recommended to add events and indexed events at all possible function calls for better logging.

Low Severity findings:

Title:

Unchecked return value in transfer

Description:

Several tokens do not revert in case of failure and return false if the return value is not handled. Any error which caused the failure on transfer will go unnoticed.

PoC:

• Go to the below lines of code 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
• We will notice the transfer call's return value is not handled anywhere later.

Suggested Fix:

Consider the use of safeTransfer instead of transfer that auto asserts and handles in case of transfer failure.

Title:

Unused return

Description:

The return value of an external call is not stored in a local or state variable which can cause improper assumptions as the return value was not handled/processed.

PoC:

• Go to the below lines https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L243 We will notice the return value of deposit call is not stored and handled.

• Also, at line 79-80 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L79-L80 We will notice the return value of the approve function call is not used or handled.

Suggested Fix:

Ensure that all the return values of the function calls are used.

Title:

Multiple functions Lacking Zero address checks

Description:

Address type parameters should include a zero-address check; otherwise contract functionality may become inaccessible, or tokens burned forever.

Impact

Tokens may become inaccessible or burnt forever without a zero-address check.

PoC:

Below is the list of functions lacking zero address checks

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConcurRewardPool.sol#L15-L17 Missing zero address checks in _notifier

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L69-L72 Missing zero address checks in _treasury

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L82-L84 Missing zero address checks in _treasury

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L86-L88 Missing zero address checks in _claimContract

• https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L78-L80 Missing zero address checks in _depositor

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol#L37-L47 Missing zero address checks in _rewardsDistribution _rewardsToken & _stakingToken

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/VoteProxy.sol#L11-L13 Missing zero address checks in _auctioneer

Suggested Fix:

Address zero address check to all the missing places.

Non-critical Severity findings:

Title:

Meaningless comparison to boolean constant

Description:

Comparison to boolean constant is meanless and not required. Boolean constants can be used directly and do not need to be compared to true or false.

PoC:

• Go to https://github.com/code-423n4/2022-02-concur/blob/main/contracts/VoteProxy.sol#L21 And we will notice if (auctioneer.isWinningSignature(_hash, _signature) == true) which can simply be if (auctioneer.isWinningSignature(_hash, _signature)

Suggested Fix:

Use if (auctioneer.isWinningSignature(_hash, _signature) instead.

Title:

Multiple missing events in critical functions

Description:

Events are important and should be emitted for tracking this off-chain for all important functions.

PoC:

The below functions are missing events.

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

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

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

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

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L94-L96 (Need to check how it works)

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

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L78-L80

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

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

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/VoteProxy.sol#L11-L13

Suggested Fix:

Address zero address check to all the missing places.

Summary

During the code review, we found that few state variables that were declared as private could be marked as constant as they were not expected to be updated. Constant variables consume lesser gas than private variables. Also, we found a few functions that were declared as public but were never internally used. Such functions can always be marked as external instead of public as external functions consume lesser gas in comparison to public functions. It was also noticed that the ordering of variables in structs was in random order. Structs perform variable packing, and hence it is recommended to go from lower bytes to higher bytes as the lower bytes get packed together to save gas. The contract was also found to be using the safeMath library, which is redundant if version solidity compiler version 0.8.0 and above is used as it has enough built-in features to prevent buffer overflow attacks and perform safer arithmetic calculations. It is advised to remove the safeMath library as they are super expensive in terms of gas consumption.

Title:

State variables that could be declared constant

Description:

Constant state variables should be declared constant to save gas.

PoC:

The below variables concurPerBlock _concurShareMultiplier _perMille can be used as constants instead of private. https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L50 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L56 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L57

Suggested Fix:

concurPerBlock _concurShareMultiplier _perMille can be used as constants instead of private to save gas.

Title:

Functions that can be external instead of public

Description:

Public functions that are never called by a contract should be declared external in order to conserve gas.

Impact

Smart Contracts are required to have effective Gas usage as they cost real money, and each function should be monitored for the amount of gas it costs to make it gas efficient.

Public functions cost more Gas than external functions.

PoC:

The following functions can be declared external: https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L93-L118 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L86-L101 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L127-L132

Suggested Fix:

Use the external state visibility for functions that are never called from the contract.

Title:

Ordering of structs

Description:

Structs are packed, and arrangement matters for packing, which in turn affects the gas used. The ordering should be from lower to higher space consumption so packing can be done.

PoC:

• Go to https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L38-L44
• We will notice the random ordering of structs which will cost more gas

Suggested Fix:

Use the ordering as below, switching depositFeeBP to 2nd place instead of last.

IERC20 depositToken; // Address of LP token contract. uint16 depositFeeBP; // Deposit fee in basis points uint allocPoint; // How many allocation points assigned to this pool. to distribute per block. uint lastRewardBlock; // Last block number that distribution occurs. uint accConcurPerShare; // Accumulated per share, times multiplier. See below.

Title:

Use of Safemath

Description:

Safemath module is considered very gas expensive. It was useful before solidity 0.8.0. After the 0.8.0 version, solidity auto handles buffer overflow, and hence safeMath can be avoided.

PoC:

The MasterChef.sol file is using safeMath which is https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L10

Suggested Fix:

Avoid using safeMath module to save gas