The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
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.
vToken.sol, mint
AUMCalculationLibrary.sol, rpow
FullMath.sol, mulDiv
vToken.sol, burn
Title: Assert instead require to validate user inputs
Severity: Low Risk
From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.
IndexLogic.sol : reachable assert in line 71
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.
Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.
Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself.
(details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64)
The vulnerable initialization functions in the codebase are:
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.
PhuturePriceOracle.sol.setOracleOf _asset
IndexLogic.sol.burn _recipient
vToken.sol.transferFrom _to
vToken.sol.transfer _recipient
vToken.sol.transferFrom _from
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: Does not validate the input fee parameter
Severity: Low Risk
Some fee parameters of functions are not checked for invalid values. Validate the parameters:
PhutureIndex._chargeAUMFee (_feePool)
Title: Require with empty message
Severity: Low Risk
The following requires are with empty messages.
This is very important to add a message for any require. Such that the user has enough
information to know the reason of failure:
Solidity file: FullMath.sol, In line 44 with Empty Require message.
Title: Missing commenting
Severity: Low Risk
The following functions are missing commenting as describe below:
NAV.sol, transfer (internal), parameter self not commented
moose-code
commented
2 years ago
Title: Solidity compiler versions mismatch Severity: Low Risk
The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
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: Assert instead require to validate user inputs Severity: Low Risk
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: Init frontrun Severity: Low Risk
Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.
Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:
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: Does not validate the input fee parameter Severity: Low Risk
Some fee parameters of functions are not checked for invalid values. Validate the parameters:
Title: Require with empty message Severity: Low Risk
The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:
Title: Missing commenting Severity: Low Risk