Badger Citadel contest - SolidityScan's results

Summary:

During the code assessment, we found multiple issues related to input validation in case 0 is passed as the value. This could lead to unexpected results in calculations. We also found that the required statement made an external call with a state change. Require statements should be used only for error validation and not state changes. Another issue was related to missing events in the critical function setting admin roles. It is important to emit events for these functions.

Low Severity findings:

QA - 1

Title:

Missing input validation in amounts

Description:

The following functions were missing an input validation in the amounts. They do not check if the amount value is set to zero.

1. mintAndDistribute()
2. _transferToFundingPools()

Impact

Due to unforeseen circumstances or errors introduced in the code logic due to user inputs, default values, or other scenarios the amount value may be set to zero which will cause the functions to fail and may cause loss of funds.

PoC:

• Notice the function mintAndDistribute() using the variable mintable at lines https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L191 and https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L215.
• This variable is coming from supplySchedule.getMintable(cachedLastMintTimestamp); at line https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L177
• Another vulnerable instance is in function _transferToFundingPools on lines https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L338-L360.
• The vulnerable parameter is _citadelAmount which is being used at line https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L348 without any input validation.

Suggested Fix:

Validate the amounts to check if their values are being set to 0 using require statements.

QA - 2

Title:

Misconfigured Require statements

Description:

Require statements should only be used to validate inputs. Invariants in the require() statements should not modify the state per best practices. The functions _removeFundingPool and _addFundingPool were found to be using require statements in which external functions were called.

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 the lines at https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L369 and https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L375-L376
• It can be seen that the require statements are making external calls to fundingPools.remove(_pool) and fundingPools.add(_pool) and therefore making state changes.

Suggested Fix:

require() statements should only be used for checking error conditions on inputs and return values. They should not be making external calls or changes to the state.

Non-critical findings

QA - 3

Title:

Missing events in critical function

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 function, 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-04-badger-citadel/blob/main/src/GlobalAccessControl.sol#L107-L122The 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.

Summary

Struct packings save gas by packing in slots. This can be optimized by arranging the structs in a manner that they get packed and consume less gas.

Gas-1

Title:

Structs packing to save Gas

Description:

If the variable types in the struct are arranged in an orer such that all the uint256 variable types are kept before the address type, the gas used is lesser than when they are arranged in any other order. The main goal here is the reduction of gas requirements when deploying these smart contracts, and saving costs in each place will eventually add up. The consequences of the use of the Tight Variable Packing pattern have to be evaluated before implementing it blindly. The big benefit comes from the substantial amount of gas that can potentially be saved over the lifetime of a contract.

PoC:

At line (https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/Funding.sol#L46-L53) We will notice that the structs are ordered as below

uint256 discount;
uint256 minDiscount;
uint256 maxDiscount;
address discountManager;
uint256 assetCumulativeFunded; /// persistent sum of asset amount in over lifetime of contract.
uint256 assetCap;

This will consume 272337 gas when tested in a seperate contract.

However, when the struct is arranged in following order, keeping the address variable type in the end, will consume lower gas - 272325.

uint256 discount;
uint256 minDiscount;
uint256 maxDiscount;
uint256 assetCumulativeFunded;
uint256 assetCap;
address discountManager;

The above arrangement will consume 272325 gas

Suggested Fix:

Arrange the struct variables in the order mentioned in the PoC, keeping the address type variable in the end.

uint256 discount;
uint256 minDiscount;
uint256 maxDiscount;
uint256 assetCumulativeFunded;
uint256 assetCap;
address discountManager;