Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #358

Open
code423n4 opened this issue Aug 6, 2022 · 1 comment
Open

QA Report #358

code423n4 opened this issue Aug 6, 2022 · 1 comment
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

Comments

@code423n4
Copy link
Contributor

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:

  • 65
  • 27
  • 28
  • 1000
  • 86400
  • 365000

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:

Mitigation:

Comments and lines of code should be wrapped to a maximum of 79 (or 99) characters to help readers easily parse the comments.

@code423n4 code423n4 added 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 labels Aug 6, 2022
code423n4 added a commit that referenced this issue Aug 6, 2022
@0xA5DF
Copy link

0xA5DF commented Aug 11, 2022

Front run initializer is dupe of #6 which was filled as HIGH severity (writing this in case it's accepted as higher then low/qa, otherwise it doesn't matter)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

No branches or pull requests

3 participants