Rigor Protocol contest - oyc_109's results

[L-01] Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Community.sol::440 => .lastTimestamp = block.timestamp;
Community.sol::685 => uint256 _noOfDays = (block.timestamp -
Community.sol::847 => _communityProject.lastTimestamp = block.timestamp;

[L-02] Missing Zero address checks

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters. Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

HomeFi.sol::119 => trustedForwarder = _forwarder;
HomeFi.sol::206 => trustedForwarder = _newForwarder;
Project.sol::100 => homeFi = IHomeFi(_homeFiAddress);
Project.sol::103 => builder = _sender;
Project.sol::104 => currency = IDebtToken(_currency);

[L-03] Events not emitted for important state changes

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

HomeFi.sol::200 => function setTrustedForwarder(address _newForwarder)

[L-04] ecrecover() not checked for signer address of zero

The solidity function ecrecover is used, however the error result of 0 is not checked for. See documentation: https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions "recover the address associated with the public key from elliptic curve signature or return zero on error. "

Recommended Mitigation Steps:

Verify that the result from ecrecover isn't 0

SignatureDecoder.sol::39 => return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);

[L-05] _safeMint() should be used rather than _mint() wherever possible

If the _to account is a contract that cannot handle ERC721 tokens, using _mint could result in the NFT being stuck. Recommend using _safeMint() instead.

DebtToken.sol::66 => _mint(_to, _total);
HomeFi.sol::292 => _mint(_to, projectCount);

[L-06] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

__gap is empty reserved space in storage that is recommended to be put in place in upgradeable contracts. It allows new state variables to be added in the future without compromising the storage compatibility with existing deployments

Community.sol::8 => import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
DebtToken.sol::6 => import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
Disputes.sol::8 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
HomeFiProxy.sol::5 => import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
HomeFi.sol::7 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
ProjectFactory.sol::8 => import {ClonesUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/ClonesUpgradeable.sol";
Project.sol::9 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

[L-07] Implementation contract may not be initialized

Implementation contract does not have a constructor with the initializer modifier therefore may be uninitialized. Implementation contracts should be initialized to avoid potential griefs or exploits.

Community.sol::8 => import {PausableUpgradeable} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
DebtToken.sol::6 => import {ERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
Disputes.sol::8 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
HomeFiProxy.sol::5 => import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
HomeFi.sol::7 => import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
ProjectFactory.sol::8 => import {ClonesUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/ClonesUpgradeable.sol";

[L-08] Upgrade Open Zeppelin contract dependency

An outdated OZ version is used (which has known vulnerabilities, see https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories).

The solution uses:

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

[L-09] No Transfer Ownership Pattern

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

HomeFi.sol

    function replaceAdmin(address _newAdmin)
        external
        override
        onlyAdmin
        nonZero(_newAdmin)
        noChange(admin, _newAdmin)
    {
        // Replace admin
        admin = _newAdmin;

        emit AdminReplaced(_newAdmin);
    }

HomeFiProxy.sol

    function changeProxyAdminOwner(address _newAdmin)
        external
        onlyOwner
        nonZero(_newAdmin)
    {
        // Transfer ownership to new admin.
        proxyAdmin.transferOwnership(_newAdmin);
    }

[L-10] Fees can be modified without timelock and upperbound check

The lender fee in HomeFi.sol can be modified by the admin by calling replaceLenderFee(), this function has no upper bound check nor timelock.

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L185-L197

This could lead to front-running either on purpose or accidentally to cause a higher fee to be paid to the homeFi.treasury(), and directly reduces the amount going to project

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L397 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L443

Recommendation: add upper bound check and timelock to critical setters

[L-11] Same function defined in two locations

Both Project.sol and Community.sol have the function checkSignatureValidity() which repeats the same code only with a different revert string

It is not recommended to repeat code in two separate locations as this could lead to errors.

Recommend refactoring this function into a library contract

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L875-L892 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L871-L893

[L-12] Checks should be added for signature malleability

The function recoverKey() returns the value from ecrecover() without using a nonce, this could lead to signature replay attacks

SignatureDecoder.sol::39 => return ecrecover(toEthSignedMessageHash(messageHash), v, r, s);

Recommended Mitigation Steps

Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly

[N-01] Use a more recent version of solidity

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

Community.sol::3 => pragma solidity 0.8.6;
DebtToken.sol::3 => pragma solidity 0.8.6;
Disputes.sol::3 => pragma solidity 0.8.6;
HomeFiProxy.sol::3 => pragma solidity 0.8.6;
HomeFi.sol::3 => pragma solidity 0.8.6;
ProjectFactory.sol::3 => pragma solidity 0.8.6;
Project.sol::3 => pragma solidity 0.8.6;
SignatureDecoder.sol::3 => pragma solidity 0.8.6;
Tasks.sol::3 => pragma solidity 0.8.6;

[N-02] Constants should be defined rather than using magic numbers

It is bad practice to use numbers directly in code without explanation

Community.sol::394 => (_projectInstance.lenderFee() + 1000);
Community.sol::694 => 365000;
Project.sol::907 => ((_amount / 1000) * 1000) == _amount,
SignatureDecoder.sol::25 => if (messageSignatures.length % 65 != 0) {
SignatureDecoder.sol::35 => if (v != 27 && v != 28) {
SignatureDecoder.sol::82 => if (v < 27) {
SignatureDecoder.sol::83 => v += 27;

[N-03] Adding a return statement when the function defines a named return variable, is redundant

It is not necessary to have both a named return and a return statement.

Community.sol::903 => returns (address sender)
Community.sol::914 => returns (bytes calldata)
HomeFi.sol::307 => returns (address sender)
HomeFi.sol::318 => returns (bytes calldata)
Project.sol::720 => returns (bool[3] memory _alerts)
Tasks.sol::194 => returns (uint256 _state)

[N-04] Missing event for critical parameter change

Emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following changes to the contract.

HomeFi.sol::200 => function setTrustedForwarder(address _newForwarder)

[N-05] Use native time units such as seconds, minutes, hours, days, weeks and years, rather than numbers of days

Community.sol::694 => 365000;

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

Community.sol::624 => for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
HomeFiProxy.sol::87 => for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol::136 => for (uint256 i = 0; i < _length; i++) {
Project.sol::248 => for (uint256 i = 0; i < _length; i++) {
Project.sol::311 => for (uint256 i = 0; i < _length; i++) {
Project.sol::322 => for (uint256 i = 0; i < _length; i++) {
Tasks.sol::181 => for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

[G-02] Cache Array Length Outside of Loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Project.sol::603 => for (; i < _changeOrderedTask.length; i++) {

[G-03] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0. This change saves 6 gas per instance

Community.sol::764 => require(_repayAmount > 0, "Community::!repay");
Project.sol::195 => require(_cost > 0, "Project::!value>0");

[G-04] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Community.sol::555 => function restrictToAdmin() external override onlyHomeFiAdmin {
Community.sol::566 => function unrestrictToAdmin() external override onlyHomeFiAdmin {
Community.sol::577 => function pause() external override onlyHomeFiAdmin {
Community.sol::582 => function unpause() external override onlyHomeFiAdmin {

[G-05] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Project.sol::88 => constructor() initializer {}

[G-06] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

DebtToken.sol::16 => uint8 internal _decimals;
SignatureDecoder.sol::29 => uint8 v;

[G-07] Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. Use uint256(1) and uint256(2) for true/false instead

Community.sol::55 => bool public override restrictedToAdmin;
Community.sol::61 => mapping(address => mapping(bytes32 => bool)) public override approvedHashes;
HomeFi.sol::50 => bool public override addrSet;
Project.sol::68 => bool public override contractorConfirmed;
Project.sol::78 => bool public override contractorDelegated;
Project.sol::84 => mapping(address => mapping(bytes32 => bool)) public override approvedHashes;

[G-08] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

Community.sol::140 => communityCount++;
Community.sol::266 => _community.publishNonce = ++_community.publishNonce;
Community.sol::624 => for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
Disputes.sol::121 => emit DisputeRaised(disputeCount++, _reason);
HomeFiProxy.sol::87 => for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol::136 => for (uint256 i = 0; i < _length; i++) {
Project.sol::248 => for (uint256 i = 0; i < _length; i++) {
Project.sol::311 => for (uint256 i = 0; i < _length; i++) {
Project.sol::322 => for (uint256 i = 0; i < _length; i++) {
Project.sol::368 => for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
Project.sol::603 => for (; i < _changeOrderedTask.length; i++) {
Project.sol::625 => _loopCount++;
Project.sol::650 => for (++j; j <= taskCount; j++) {
Project.sol::672 => _loopCount++;
Project.sol::710 => for (uint256 _taskID = 1; _taskID <= _length; _taskID++) {
Tasks.sol::181 => for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

[G-09] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

use <x> = <x> + <y> or <x> = <x> - <y> instead to save gas

Community.sol::423 => .totalLent += _amountToProject;
Community.sol::435 => .lentAmount += _lendingAmount;
Community.sol::798 => _interest -= _repayAmount;
HomeFi.sol::289 => projectCount += 1;
Project.sol::179 => hashChangeNonce += 1;
Project.sol::250 => _taskCount += 1;
Project.sol::290 => hashChangeNonce += 1;
Project.sol::431 => totalAllocated -= _withdrawDifference;
Project.sol::440 => totalAllocated += _newCost - _taskCost;
Project.sol::456 => totalAllocated -= _taskCost;
Project.sol::616 => _costToAllocate -= _taskCost;
Project.sol::663 => _costToAllocate -= _taskCost;
Project.sol::711 => _cost += tasks[_taskID].cost;
Project.sol::772 => totalLent -= _amount;
SignatureDecoder.sol::83 => v += 27;

[G-10] Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Community.sol::69 => require(_address != address(0), "Community::0 address");
Community.sol::75 => require(_msgSender() == homeFi.admin(), "Community::!admin");
Community.sol::241 => require(homeFi.isProjectExist(_project), "Community::Project !Exists");
Community.sol::248 => require(_community.isMember[_builder], "Community::!Member");
Community.sol::536 => require(_builder == _projectInstance.builder(), "Community::!Builder");
Community.sol::557 => require(!restrictedToAdmin, "Community::restricted");
Community.sol::568 => require(restrictedToAdmin, "Community::!restricted");
Community.sol::764 => require(_repayAmount > 0, "Community::!repay");
Community.sol::792 => require(_lentAndInterest >= _repayAmount, "Community::!Liquid");
DebtToken.sol::50 => require(_communityContract != address(0), "DebtToken::0 address");
Disputes.sol::39 => require(_address != address(0), "Disputes::0 address");
Disputes.sol::46 => require(homeFi.admin() == _msgSender(), "Disputes::!Admin");
Disputes.sol::52 => require(homeFi.isProjectExist(_msgSender()), "Disputes::!Project");
Disputes.sol::183 => require(_result, "Disputes::!Member");
HomeFiProxy.sol::41 => require(_address != address(0), "Proxy::0 address");
HomeFiProxy.sol::81 => require(_length == _implementations.length, "Proxy::Lengths !match");
HomeFiProxy.sol::133 => require(_length == _contractAddresses.length, "Proxy::Lengths !match");
HomeFi.sol::73 => require(admin == _msgSender(), "HomeFi::!Admin");
HomeFi.sol::78 => require(_address != address(0), "HomeFi::0 address");
HomeFi.sol::84 => require(_oldAddress != _newAddress, "HomeFi::!Change");
HomeFi.sol::142 => require(!addrSet, "HomeFi::Set");
HomeFi.sol::191 => require(lenderFee != _newLenderFee, "HomeFi::!Change");
ProjectFactory.sol::36 => require(_address != address(0), "PF::0 address");
ProjectFactory.sol::84 => require(_msgSender() == homeFi, "PF::!HomeFiContract");
Project.sol::123 => require(!contractorConfirmed, "Project::GC accepted");
Project.sol::132 => require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol::135 => require(_contractor != address(0), "Project::0 address");
Project.sol::150 => require(_msgSender() == builder, "Project::!B");
Project.sol::153 => require(contractor != address(0), "Project::0 address");
Project.sol::176 => require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol::195 => require(_cost > 0, "Project::!value>0");
Project.sol::238 => require(_taskCount == taskCount, "Project::!taskCount");
Project.sol::241 => require(_projectAddress == address(this), "Project::!projectAddress");
Project.sol::245 => require(_length == _taskCosts.length, "Project::Lengths !match");
Project.sol::277 => require(_nonce == hashChangeNonce, "Project::!Nonce");
Project.sol::308 => require(_length == _scList.length, "Project::Lengths !match");
Project.sol::341 => require(_projectAddress == address(this), "Project::!Project");
Project.sol::369 => require(tasks[_taskID].getState() == 3, "Project::!Complete");
Project.sol::406 => require(_project == address(this), "Project::!projectAddress");
Project.sol::511 => require(_project == address(this), "Project::!projectAddress");
Project.sol::530 => require(getAlerts(_task)[2], "Project::!SCConfirmed");
Project.sol::753 => require(_sc != address(0), "Project::0 address");
Tasks.sol::44 => require(_self.state == TaskStatus.Inactive, "Task::active");
Tasks.sol::50 => require(_self.state == TaskStatus.Active, "Task::!Active");
Tasks.sol::124 => require(_self.subcontractor == _sc, "Task::!SC");

[G-11] Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

Community.sol::3 => pragma solidity 0.8.6;
DebtToken.sol::3 => pragma solidity 0.8.6;
Disputes.sol::3 => pragma solidity 0.8.6;
HomeFiProxy.sol::3 => pragma solidity 0.8.6;
HomeFi.sol::3 => pragma solidity 0.8.6;
ProjectFactory.sol::3 => pragma solidity 0.8.6;
Project.sol::3 => pragma solidity 0.8.6;
SignatureDecoder.sol::3 => pragma solidity 0.8.6;
Tasks.sol::3 => pragma solidity 0.8.6;

[G-12] Prefix increments cheaper than Postfix increments

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) Saves 5 gas PER LOOP

Community.sol::624 => for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
HomeFiProxy.sol::87 => for (uint256 i = 0; i < _length; i++) {
HomeFiProxy.sol::136 => for (uint256 i = 0; i < _length; i++) {
Project.sol::248 => for (uint256 i = 0; i < _length; i++) {
Project.sol::311 => for (uint256 i = 0; i < _length; i++) {
Project.sol::322 => for (uint256 i = 0; i < _length; i++) {
Project.sol::603 => for (; i < _changeOrderedTask.length; i++) {
Tasks.sol::181 => for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];

[G-13] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

DebtToken.sol::82 => function decimals() public view virtual override returns (uint8) {

[G-14] Not using the named return variables when a function returns, wastes deployment gas

It is not necessary to have both a named return and a return statement.

Community.sol::903 => returns (address sender)
Community.sol::914 => returns (bytes calldata)
HomeFi.sol::307 => returns (address sender)
HomeFi.sol::318 => returns (bytes calldata)
Project.sol::720 => returns (bool[3] memory _alerts)
Tasks.sol::194 => returns (uint256 _state)

[G-15] Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

Community.sol::59 => mapping(address => uint256) public override projectPublished;
Community.sol::61 => mapping(address => mapping(bytes32 => bool)) public override approvedHashes;
HomeFi.sol::64 => mapping(address => uint256) public override projectTokenId;
HomeFi.sol::66 => mapping(address => address) public override wrappedToken;

[G-16] Use assembly to check for address(0)

Saves 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

instances:

Community.sol::69 => require(_address != address(0), "Community::0 address");
DebtToken.sol::50 => require(_communityContract != address(0), "DebtToken::0 address");
Disputes.sol::39 => require(_address != address(0), "Disputes::0 address");
HomeFiProxy.sol::41 => require(_address != address(0), "Proxy::0 address");
HomeFi.sol::78 => require(_address != address(0), "HomeFi::0 address");
ProjectFactory.sol::36 => require(_address != address(0), "PF::0 address");
Project.sol::135 => require(_contractor != address(0), "Project::0 address");
Project.sol::153 => require(contractor != address(0), "Project::0 address");
Project.sol::478 => if (_newSC != address(0)) {
Project.sol::753 => require(_sc != address(0), "Project::0 address");

[G-17] internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Disputes.sol::207 => function resolveHandler(uint256 _disputeID) internal {
Project.sol::770 => function autoWithdraw(uint256 _amount) internal {

[G-18] internal functions not called by the contract should be removed to save deployment gas

If the functions are required by an interface, the contract should inherit from that interface and use the override keyword

Tasks.sol::138 => function fundTask(Task storage _self) internal {
Tasks.sol::148 => function unAllocateFunds(Task storage _self) internal {
Tasks.sol::160 => function unApprove(Task storage _self) internal {