QA Report #358
Labels
bug
Something isn't working
old-submission-method
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
valid
QA
Low
Use of hardcoded amount of days
Summary
Formula uses a hardcoded value of 365 (days) which would be wrong applied in a leap year (366 days)
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/Community.sol#L688-L694
Mitigation
Consider using an oracle for this
Consider using a method that change the value between 365 and 366 for the operations in leap years and regular years
_safeMint() should be used rather than _mint() wherever possible
Summary
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.
Details
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L271
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L292
Mitigation
Use _safeMint() wherever possible
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccack256()
Summary
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456) , but abi.encode(0x123,0x456) => 0x0...1230...456 ).
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L50
Mitigation
“Unless there is a compelling reason, abi.encode should be preferred”.
If there is only one argument to abi.encodePacked() it can often be cast to
bytes() or bytes32() instead.
Front run initializer
Summary
The initialize function that initializes important contract state can be called by anyone.
Details
The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract.
In the best case for the victim, they notice it and have to redeploy their contract costing gas.
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/ProjectFactory.sol#L44-L55
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/Community.sol#L102-L119
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/DebtToken.sol#L43-L58
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Disputes.sol#L74-L81
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L92-L120
Mitigation
Use the constructor to initialize non-proxied contracts.
For initializing proxy contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.
Missing checks for address(0x0) when assigning values to address state variables
Summary
Zero address should be checked for state variables and some parameters in functions like mints, withdrawals... A zero address can lead into problems.
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L96
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L98
Mitigation
Check zero address for state variables before assigning to them a value
Informational
Missing event on interface
Summary:
transfer and transferFrom from ERC20 expect to emit Transfer event, however, in this ERC20 implementation they are missing
Details
This two methods are blocked with a revert condition, however, ERC20 expects them to be there.
transfer -> Must emit be view Transfer(address,address,uint256)
transferFrom -> Must emit be view Transfer(address,address,uint256)
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/DebtToken.sol#L90-L105
Mitigation
Consider to follow the ERC20 interface in his totality, or save gas on deployment, keeping it as it is implemented right now
Use of magic numbers is confusing and risky
Summary:
Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.
Details:
Values are hardcoded and would be more readable and maintainable if declared as a constant
Following magic numbers are used:
Github Permalinks:
27 -> pretty good commented
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L82-L83
27, 28 -> not commented
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L35
65 -> not commented
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L25
86400 -> pretty good commented
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/Community.sol#L686
365000 -> pretty good commented
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/Community.sol#L694
1000 -> pretty good commented
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L907
Mitigation:
Define constants for the numbers used throughout the code and use the comments already written / write comments for them
Missing indexed event parameters
Summary:
Events without indexed event parameters make it harder and
inefficient for off-chain tools to analyze them.
Details:
Indexed parameters (“topics”) are searchable event parameters.
They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.
Github Permalinks:
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/interfaces/IHomeFi.sol#L15-L17
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/interfaces/IHomeFi.sol#L25
Mitigation:
Consider which event parameters could be particularly useful to off-chain tools and should be indexed.
Unused code
Summary:
Code that is not used should be removed
Github Permalinks:
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Community.sol#L910-L918
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L314-L322
Mitigation:
Remove the code that is not used.
Missing Natspec
Summary:
Missing Natspec and regular comments affect readability and maintainability of a codebase.
Details:
Contracts has partial or full lack of comments
Github Permalinks:
Natspec @param and @return
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/DebtToken.sol
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/ProjectFactory.sol
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Disputes.sol#L26-L194
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/Community.sol#L33-L716
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L51-L733
Some comments and nastpec/@return
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L43-L84
Mitigation
Add @param descriptors
Add @return descriptors
Add Natspec comments.
Add comments for what the contract does
Inconsistent spacing in comments
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file
Details
uint256 public override disputeCount; //starts from 0
But following the style of the other comments would be:
uint256 public override disputeCount; // starts from 0
Github Permalinks
Maximum line length exceeded
Summary:
Long lines should be wrapped to conform with Solidity Style guidelines.
Details:
Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Github Permalinks:
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/Tasks.sol#L39
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L56
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/libraries/SignatureDecoder.sol#L58
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L50
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L56
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L123
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L785
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L819
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L823
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/Project.sol#L824
Mitigation:
Comments and lines of code should be wrapped to a maximum of 79 (or 99) characters to help readers easily parse the comments.
The text was updated successfully, but these errors were encountered: