Either use the named return variables or remove them.
5- Related data should be grouped in a struct :
When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.
Impact - NON CRITICAL
Proof of Concept
Instances include:
File: src/policies/Governance.sol
96 mapping(uint256 => ProposalMetadata) public getProposalMetadata;
99 mapping(uint256 => uint256) public totalEndorsementsForProposal;
102 mapping(uint256 => mapping(address => uint256)) public userEndorsementsForProposal;
105 mapping(uint256 => bool) public proposalHasBeenActivated;
108 mapping(uint256 => uint256) public yesVotesForProposal;
111 mapping(uint256 => uint256) public noVotesForProposal;
114 mapping(uint256 => mapping(address => uint256)) public userVotesForProposal;
117 mapping(uint256 => mapping(address => bool)) public tokenClaimsForProposal;
Mitigation
Group the related data in a struct and use one mapping:
While all are technically valid and the named return is useful, checking zero addresses, setter checks, etc are unnecessary additions in a permissioned system where we can control the inputs.
QA Report
Summary
address(0)
checksFindings
1- Missing zero address
address(0)
checks :Constructors should check the address written in an immutable address variable is not the zero address
Impact - Low Risk
Proof of Concept
Instances include:
File: src/Kernel.sol
kernel = kernel_;
File: src/modules/MINTR.sol
ohm = OHM(ohm_);
File: src/modules/RANGE.sol
ohm = tokens_[0];
reserve = tokens_[1];
File: src/policies/Operator.sol
ohm = tokens_[0];
reserve = tokens_[1];
Mitigation
Add non-zero address checks in the constructors for the instances aforementioned.
2- Setters should check the input value and revert if it's the zero address or zero :
Setters should check the input values and revert if it's the zero address or zero.
Impact - Low Risk
Proof of Concept
Instances include:
File: src/Kernel.sol
function changeKernel(Kernel newKernel_)
File: src/policies/Heart.sol
function setRewardTokenAndAmount(ERC20 token, uint256 reward)
Mitigation
Add non-zero checks - address or uint - to the setters aforementioned.
3- Event should be emitted in setters :
Setters should emit an event so that Dapps can detect important changes to storage.
Impact - Low Risk
Proof of Concept
Instances include:
File: src/Kernel.sol
function changeKernel(Kernel newKernel_)
function setActiveStatus(bool activate_)
File: src/policies/BondCallback.sol
function setOperator(Operator operator_)
File: src/policies/Operator.sol
function setBondContracts(IBondAuctioneer auctioneer, IBondCallback callback)
File: src/policies/VoterRegistration.sol
function issueVotesTo(address wallet, uint256 amount)
function revokeVotesFrom(address wallet, uint256 amount)
Mitigation
Emit an event in the functions aforementioned.
4- Named return variables not used anywhere in the function :
Named return variable should be used inside the function or if not they should be removed to avoid confusion.
Impact - NON CRITICAL
Proof of Concept
Instances include:
File: src/policies/BondCallback.sol
returns (uint256 in, uint256 out)
Mitigation
Either use the named return variables or remove them.
5- Related data should be grouped in a struct :
When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.
Impact - NON CRITICAL
Proof of Concept
Instances include:
Mitigation
Group the related data in a struct and use one mapping:
And it would be used as a state variable :