Redacted Cartel contest - SolidityScan's results

Lines of code

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L164-L205

Vulnerability details

Description

The contract was found vulnerable to Reentrancy attack. It was noticed that the function depositBribeERC20 makes an external call to another untrusted address or a contract before it resolves any effects at line "https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L187"

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.

• This vulnerability may lead to addition of more tokens in bribe than actual contribution.

PoC

• Go to function depositBribeERC20 at https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L164-L205
• We will notice the function is lacking nonReentrant modifier

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 call like "transfer," "deposit," "approve," etc., returns 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, it will be unrecoverable. The contract was found to be using floating pragma, which allows the contract to be compiled to multiple versions and hence can cause inconsistencies or errors on different versions. We also noticed the uses of large number literals. Although it is not a security issue but enhances readability and reduces the chance of errors or missing digits. 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. Lastly, we also noticed that the fee value, which is set by an admin, can be set to zero due lack of proper input validation.

Low Severity findings:

QA - 1

Title:

Unchecked return value in transfer

Description:

The return value of the token transfers is not checked or validated at all in the functions shown below. This may lead to issues if there's a critical contract logic happening below the transfer that affects or manipulates the number of ether or tokens. Therefore, if the return value is not checked, the adversaries will be able to call those functions without actually transferring any ether and manipulating the token balance.

PoC:

Transfer: Go to the below lines of code and we will notice that transfer https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L296 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L297 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L337

https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L146 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L164

Suggested Fix:

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

QA - 2

Title:

Unused return in approve call

Description:

The contract was found to be making an external call (approve). The function which is called is returning some value which is never used. This may lead to discrepancies and improper assumptions in the calling function.

PoC:

• Go to the lines below and we will notice that the approve function's return value is not used for any further validation or check. https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L68 https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L69

Suggested Fix:

It is recommended to make use of the return values coming from the external function that is called to make sure that the calculations following the external call are correct.

QA - 3

Title:

Use of Floating Pragma Version

Description:

The contract was found to be using a floating pragma which is not considered safe as it can be compiled with all the versions described.

PoC:

Pragma version^0.8.0 (https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L2) Pragma version^0.8.0 (https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/RewardDistributor.sol#L2) Pragma version^0.8.0 (https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L2) Pragma version^0.8.0 (https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L2)

Suggested Fix:

Use strict pragma version like Pragma version 0.8.0

QA - 4

Title:

Missing input validation in setFee function

Description:

The function setFee does not check if the fee value is set to zero.

Impact

An admin can set fee value to zero by mistake, and it can cause free trades and loss to the organization.

PoC:

• Notice the function below https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L108-L113
• The setFee function just checks if the fee is less than feeDivisor but does not check if the fee is zero.

Suggested Fix:

Check if the fee is being set to zero.

Non-critical findings

QA - 5

Title:

Use of Large Number Literals

Description:

Integer literals are formed from a sequence of digits in the range 0-9. They are interpreted as decimals. The use of very large numbers with too many digits was detected in the code that could have been optimized using a different notation also supported by Solidity.

Impact

Literals with many digits are difficult to read and review. This may also introduce errors in the future if one of the zeroes is omitted while doing code modifications.

PoC:

• Go to https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/BribeVault.sol#L26 We will notice a large literal uint256 public constant feeDivisor = 1000000; where 1000000 can be represnted as 10e6

Suggested Fix:

Scientific notation in the form of 2e10 is also supported, where the mantissa can be fractional but the exponent has to be an integer. The literal MeE is equivalent to M * 10**E. Examples include 2e10, 2e10, 2e-10, 2.5e1.

Reference: https://docs.soliditylang.org/en/latest/types.html#rational-and-integer-literals

QA - 6

Title:

Multiple missing events in critical functions

Description:

Events are inheritable members of contracts. When you call them, they cause the arguments to be stored in the transaction's log, a special data structure in the blockchain. These logs are associated with the address of the contract, which can then be used by developers and auditors to keep track of the transactions.

The contract was found to be missing these events on certain critical functions, which would make it difficult or impossible to track these transactions off-chain.

Impact

Events are used to track the transactions off-chain, and missing these events on critical functions makes it difficult to audit these logs if they're needed at a later stage.

PoC:

The below functions are missing events.

• https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L76-L80 The function setSlippage is called by an admin and hence should have an event log regarding the change is slippage value.

• https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L158-L165 The withdraw function is called by an admin to withdraw funds and hence should have an event log.

• https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L108-L110 The function setRound sets a new voting round by admins and hence should have an event log.

Suggested Fix:

Consider emitting events for the functions mentioned above. It is also recommended to have the addresses indexed.

QA - 7

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-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L212-L251 Function depositBribeERC20 has address proposal that is missing zero address checks

• https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L257-L290 Function depositBribe has address input proposal, which is lacking zero address checks.

Suggested Fix:

Address zero address check to all the missing places.

Summary

We found a function that was declared as public but was 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. We also noticed the contract uses power operations at a few places instead of using the exponential form directly, which would save gas.

Gas-1

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: Function setRewardForwarding https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/TokemakBribe.sol#L298-L302

Suggested Fix:

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

Gas-2

Title:

Use of power operations instead of their exponential form

Description:

It is unnecessary to perform power operations when a number can be directly represented in an exponential form as it will save gas.

PoC:

1. At https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L103 and https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L108 10e18 can be used instead of 10**18
2. At https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L68 and https://github.com/code-423n4/2022-02-redacted-cartel/blob/main/contracts/ThecosomataETH.sol#L69 2e256 can be used instead of 2**256

Suggested Fix:

Use exponential form instead of power operations to save gas.