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.
mintAndDistribute()
_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.
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.
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.
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.
mintAndDistribute()_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:
mintAndDistribute()using the variablemintableat 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.supplySchedule.getMintable(cachedLastMintTimestamp);at line https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L177_transferToFundingPoolson lines https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/CitadelMinter.sol#L338-L360._citadelAmountwhich 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
_removeFundingPooland_addFundingPoolwere 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:
fundingPools.remove(_pool)andfundingPools.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
setSlippageis 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
withdrawfunction 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
setRoundsets 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.