QA Report #111

code423n4 · 2022-08-05T13:17:05Z

(1) Missing Checks for Address(0x0)

Severity: Low

Proof Of Concept

function _unpublishProject(address _project) internal {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L726

function setTrustedForwarder(address _newForwarder)
    external
    override
    onlyAdmin
    noChange(trustedForwarder, _newForwarder)
{

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L200

function initialize(
    address _currency,
    address _sender,
    address _homeFiAddress
) external override initializer {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L94

function initialize(
    address _currency,
    address _sender,
    address _homeFiAddress
) external override initializer {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L94

function initialize(
    address _currency,
    address _sender,
    address _homeFiAddress
) external override initializer {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L94

function recoverTokens(address _tokenAddress) external override {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L362

function createProject(address _currency, address _sender)
    external
    override
    returns (address _clone)
{

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/ProjectFactory.sol#L78

Recommended Mitigation Steps

Consider adding zero-address checks in the mentioned codebase.

(2) Transferownership Should Be Two Step

Severity: Low

The owner is the authorized user in the solidity contracts. Usually, an owner can be updated with transferOwnership function. However, the process is only completed with single transaction. If the address is updated incorrectly, an owner functionality will be lost forever.

Proof Of Concept

proxyAdmin.transferOwnership(_newAdmin);

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFiProxy.sol#L156

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

(3) Use _safeMint instead of _mint

Severity: Low

According to openzepplin's ERC721, the use of _mint is discouraged, use _safeMint whenever possible.
https://docs.openzeppelin.com/contracts/3.x/api/token/erc721#ERC721-_mint-address-uint256-

Proof Of Concept

_mint(_to, _total);

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/DebtToken.sol#L66

_mint(_to, projectCount);

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L292

(4) Update OpenZeppelin Packages to ^4.7.0

Severity: Low

According to package.json, the OZ packages are currently set to ^4.4.2

Proof Of Concept

"@openzeppelin/contracts": "^4.4.2",
"@openzeppelin/contracts-upgradeable": "^4.4.2"

https://github.com/code-423n4/2022-08-rigor/blob/main/package.json#L68-L69

Recommended Mitigation Steps

Update the versions of @openzeppelin/contracts and @openzeppelin/contracts-upgradeable to be the latest in package.json. I also recommend double checking the versions of other dependencies as a precaution, as they may include important bug fixes.

(5) Missing NATSPEC Documentation

Severity: Non-Critical

Proof Of Concept

(6) Incorrect Documentation

Severity: Non-Critical

Proof Of Concept

* @return _tokenIds NFT Id of project

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L282

Is incorrect as it returns projectCount and not _tokenIds

(7) Constants Should Be Defined Rather Than Using Magic Numbers

Severity: Non-Critical

Proof Of Concept

        (_projectInstance.lenderFee() + 1000);

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L394

    uint256 _maxLoop = 50;

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L576

        ((_amount / 1000) * 1000) == _amount,

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L907

    if (v != 27 && v != 28) {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/libraries/SignatureDecoder.sol#L35

    if (v < 27) {

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/libraries/SignatureDecoder.sol#L82

(8) Inconsistent Spacing In Comments

Severity: Non-Critical

Some lines use // x and some use //x. The instances below point out the usages that don’t follow the majority, within each file

Proof Of Concept

uint256 public override disputeCount; //starts from 0

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Disputes.sol#L29

(9) Missing event for critical parameter change

Severity: Non-Critical

Proof Of Concept

function setTrustedForwarder(

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L200

function changeProxyAdminOwner(

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFiProxy.sol#L150

function changeOrderedTask(

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L559

function changeProjectImplementation(

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/ProjectFactory.sol#L58

function setComplete(

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/libraries/Tasks.sol#L88

(10) Implementation contract may not be initialized

OpenZeppelin recommends that the initializer modifier be applied to constructors.
Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits.
https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5

Severity: Non-Critical

Proof Of Concept

contract Community is
ICommunity,
PausableUpgradeable,
ReentrancyGuardUpgradeable,
ERC2771ContextUpgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L21

contract DebtToken is IDebtToken, ERC20Upgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/DebtToken.sol#L11

contract Disputes is
IDisputes,
ReentrancyGuardUpgradeable,
ERC2771ContextUpgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Disputes.sol#L17

contract HomeFi is
IHomeFi,
ReentrancyGuardUpgradeable,
ERC721URIStorageUpgradeable,
ERC2771ContextUpgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L27

contract HomeFiProxy is OwnableUpgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFiProxy.sol#L14

contract ProjectFactory is
IProjectFactory,
Initializable,
ERC2771ContextUpgradeable

https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/ProjectFactory.sol#L16

(11) Use a more recent version of Solidity

Severity: Non-Critical

Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(,)
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

Proof Of Concept

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Community.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/DebtToken.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Disputes.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFi.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/HomeFiProxy.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/Project.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/ProjectFactory.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/libraries/SignatureDecoder.sol#L3

Found old version 0.8.6 of Solidity
https://github.com/code-423n4/2022-08-rigor/tree/main/contracts/libraries/Tasks.sol#L3

Recommended Mitigation Steps

Consider updating to a more recent solidity version.

The text was updated successfully, but these errors were encountered:

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Aug 5, 2022

code423n4 added a commit that referenced this issue Aug 5, 2022

Rolezn issue #111

87f9bf5

jack-the-pug added the valid label Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #111

QA Report #111

code423n4 commented Aug 5, 2022