Title: Missing non reentrancy modifier
Severity: Low Risk
The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer.
Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..
NonCustodialPSM.sol, withdrawERC20 is missing a reentrancy modifier
NonCustodialPSM.sol, setGlobalRateLimitedMinter is missing a reentrancy modifier
NonCustodialPSM.sol, setMintFee is missing a reentrancy modifier
NonCustodialPSM.sol, pauseRedeem is missing a reentrancy modifier
Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.
NonCustodialPSM.sol: function _validatePriceRange parameter price isn't used. (_validatePriceRange is internal)
Title: In the following public update functions no value is returned
Severity: Low Risk
Title: Two Steps Verification before Transferring Ownership
Severity: Low Risk
The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked.
It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership.
A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105
CoreRef.sol
Permissions.sol
Title: Not verified input
Severity: Low Risk
external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.
Vcon.sol.balanceOf account
PCVDeposit.sol.withdrawETH to
CoreRef.sol._mintVolt to
OracleRef.sol._setBackupOracle newBackupOracle
Permissions.sol._setupGovernor governor
Title: Check transfer receiver is not 0 to avoid burned money
Severity: Low Risk
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
owner param should be validated to make sure the owner address is not address(0).
Otherwise if not given the right input all only owner accessible functions will be unaccessible.
Volt.sol.permit owner
Vcon.sol.permit owner
Title: Named return issue
Severity: Low Risk
Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing.
Furthermore, removing either the actual return or the named return will save gas.
ScalingPriceOracle.sol, requestCPIData
Title: Missing commenting
Severity: Low Risk
The following functions are missing commenting as describe below:
NonCustodialPSM.sol, _validatePriceRange (internal), parameter price not commented
ScalingPriceOracle.sol, getCurrentOraclePrice (public), @return is missing
Permissions.sol, _setupGovernor (internal), parameter governor not commented
Volt.sol, permit (external), parameter r not commented
Title: Override function but with different argument location
Severity: Low/Med Risk
RateLimited.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
RateLimitedMinter.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
NonCustodialPSM.sol.constructor inherent OracleRef.sol.constructor but the parameters does not match
OracleRef.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
ScalingPriceOracle.sol.constructor inherent Timed.sol.constructor but the parameters does not match
NonCustodialPSM.sol.constructor inherent RateLimited.sol.constructor but the parameters does not match
NonCustodialPSM.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
Title: Add a timelock Severity: Low Risk
To give more trust to users: functions that set key/critical variables should be put behind a timelock.
Title: Missing non reentrancy modifier Severity: Low Risk
The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..
Title: Missing fee parameter validation Severity: Low Risk
Some fee parameters of functions are not checked for invalid values. Validate the parameters:
Title: Never used parameters Severity: Low Risk
Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.
Title: In the following public update functions no value is returned Severity: Low Risk
In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).
Title: Two Steps Verification before Transferring Ownership Severity: Low Risk
The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105
Title: Not verified input Severity: Low Risk
Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk
Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
Title: Not verified owner Severity: Low Risk
Title: Named return issue Severity: Low Risk
Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.
Title: Missing commenting Severity: Low Risk
Title: Override function but with different argument location Severity: Low/Med Risk