Rigor Protocol contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Disputes.sol#L90-L109 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L167 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L283-L286 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L178-L188 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L169 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L216-L226 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Community.sol#L516-L527

Vulnerability details

Impact

Neither the signed content nor the signature are associated with the contract (DOMAIN_SEPARATOR). Therefore, both can be repeated in other contracts that use similar values, usually the same builder or contractor addresses..

Proof of Concept

In some areas of the code (Project.sol#L132), the signed data contains the address of the contract, but it doesn't happend always.

// Revert if decoded project address does not match this contract. Indicating incorrect _data.
require(_projectAddress == address(this), "Project::!projectAddress");

But in the following lines, the data is not associated to the contract, so it's possible to replay the signature and the data because is not related to the contract that will consume this signature.

• Disputes.raiseDispute
• Project.updateProjectHash
• Project.updateTaskHash
• Community.addMember
• Community.addMember
• Community.publishProject
• Community.escrow

Reference:

• https://eips.ethereum.org/EIPS/eip-712

Recommended Mitigation Steps

• Use a domain separator for create the hash.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/libraries/SignatureDecoder.sol#L26-L39 https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/Project.sol#L875

Vulnerability details

Impact

The SignatureDecoder.recoverKey function can return address(0) and incur errors, so it is considered insecure.

Proof of Concept

For example, in Project.checkSignatureValidity a signature could be accepted as valid if any of the addresses used have not yet been registered (builder, _sc or contractor).

        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
        require(
            _recoveredSignature == _address || approvedHashes[_address][_hash],
            "Project::invalid signature"
        );

Affected source code:

• SignatureDecoder.sol#L26-L39

Recommended Mitigation Steps

• Revert if the signature is invalid.

• Low
  • 1. Outdated and mixed pragmas
  • 2. Upgradable contracts without GAP can lead a upgrade disaster
  • 3. Lack of ACK during owner change
  • 4. lack of checks
  • 5. Wrong comment
  • 6. Duplicate nft hash
  • 7. isPublishedToCommunity doesn't check that exists
  • 8. Denial of service by gas exhaustion
• Non-Critical
  • 9. Avoid without storage keyword in modifiers
  • 10. Unify code style
  • 11. initialize Front running
  • 12. Improve usability reducing approves
  • 13. Lack of event index
  • 14. Avoid magic numbers
  • 15. Ensure formula

Low

1. Outdated and mixed pragmas

The pragma version used are:

pragma solidity 0.8.6;
pragma solidity ^0.8.9;

Note that mixing pragma are not recommended.

The minimum required version must be 0.8.15; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

Apart from these, there are several minor bug fixes and improvements.

2. Upgradable contracts without GAP can lead a upgrade disaster

Some contract does not implement a good upgradeable logic.

Proxied contracts MUST include an empty reserved space in storage, usually named __gap, in all the inherited contracts.

For example, if it is an interface, you have to use the interface keyword, otherwise it is a contract that requires your GAP to be correctly updated, so if a new variable is created it could lead in storage collisions that will produce a contract dissaster, including loss of funds.

Reference:

• https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps
• https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/2cb8996b777060e658e2b8c9b1630313aedb04c0/contracts/token/ERC20/ERC20Upgradeable.sol#L394

Affected source code:

• DebtToken.sol#L11
• ProjectFactory.sol#L19
• HomeFiProxy.sol#L14
• Disputes.sol#L19-L20
• HomeFi.sol#L27
• Project.sol#L26-L27
• Community.sol#L23-L25

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• HomeFiProxy.sol#L150
• HomeFi.sol#L165

4. lack of checks

Check that the _data is 64 bytes length, _data is used for hashing and signature, and if it's longer than 64 bytes, the real data will stay in the fist 64 bytes, but the dummy data will create the hash, so it allows to replay the same payload without the same data.

In short, if data is concatenated at the end of data it will generate a different hash for the same payload.

• Project.inviteContractor
• Project.setComplete
• Project.changeOrder
• Project.raiseDispute

We have the same issue but with a dynamic variable, so maybe it's easier to encode the payload instead of trust in _data:

• Project.updateProjectHash
• Project.addTasks
• Project.updateTaskHash
• Project.setComplete
• Project.addMember
• Project.publishProject
• Community.escrow

lenderFee must be <= 10:

• HomeFi.sol#L115
• HomeFi.sol#L194

The following lines requires a valid address as an argument, in some of them, once the value is set, it cannot be changed again, so it is mandatory to check if these values are as expected.

Affected source code for address(0):

• Project.sol#L95-L96

5. Wrong comment

In HomeFi.sol#L115 we can read the comment:

• // the percentage must be multiplied with 10.

But the true is that the fee factor is 1000, like we can see in Community.sol#L393.

        // Calculate lenderFee
        uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
            (_projectInstance.lenderFee() + 1000);

6. Duplicate nft hash

When calling createProject an argument is passed that is used as the NFT's tokenUri, this is not checked for its existence and allows duplications between different NFTs.

Affected source code:

• HomeFi.sol#L225

7. isPublishedToCommunity doesn't check that exists

If isPublishedToCommunity is called with a _communityID = 0 and a _project that does not exist, this modifier will pass, and should not

Affected source code:

• Community.sol#L82

8. Denial of service by gas exhaustion

If a large number of members have been entered, the members method will be denied.

Affected source code:

• Community.sol#L625

---

Non-Critical

9. Avoid without storage keyword in modifiers

It is not a good practice for a modifier to make modifications, in this case it does not, but receives a variable as storage without being necessary, it is recommended to make the following change

-   modifier onlyInactive(Task storage _self) {
-       require(_self.state == TaskStatus.Inactive, "Task::active");
+   modifier onlyInactive(TaskStatus _state) {
+       require(_state == TaskStatus.Inactive, "Task::active");
        _;
    }

    /// @dev only allow active tasks. Task is inactive if SC is confirmed.
-   modifier onlyActive(Task storage _self) {
-       require(_self.state == TaskStatus.Active, "Task::!Active");
+   modifier onlyActive(TaskStatus _state) {
+       require(_state == TaskStatus.Active, "Task::!Active");
        _;
    }

Affected source code:

• Tasks.sol#L43-L61

10. Unify code style

In other methods such as replaceTreasury or replaceAdmin to verify that a change is not made, the noChange modifier is used, however in the replaceLenderFee method of HomeFi.sol#L191 is not the case and it is done inline.

    function replaceLenderFee(uint256 _newLenderFee)
        external
        override
+       noChange(lenderFee, _newLenderFee)
        onlyAdmin
    {
-       // Revert if no change in lender fee
-       require(lenderFee != _newLenderFee, "HomeFi::!Change");
- 
        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

11. initialize Front running

To initialize contract parameters, most contracts employ an initialize pattern (rather than a constructor). They are vulnerable to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attack unless they are enforced to be atomic with contact deployment via deployment script or factory contracts.

Affected source code:

• Community.sol#L102
• DebtToken.sol#L43
• Disputes.sol#L74
• HomeFi.sol#L92
• Project.sol#L94
• ProjectFactory.sol#L45

12. Improve usability reducing approves

Each transferFrom that is made requires the user to approve homeFi.treasury() and another to the _project in order to pay correctly. It is more usable for the user if the approve has him towards the community, and the community already transfers the funds at will.

-       // Transfer _lenderFee to HomeFi treasury from lender account
-       _currency.safeTransferFrom(_msgSender(), homeFi.treasury(), _lenderFee);
+       _currency.safeTransferFrom(_msgSender(), address(this), _lenderFee);
+       _currency.safeTransfer(homeFi.treasury(), _lenderFee);
+       _currency.safeTransfer(_project, _amountToProject);

-       // Transfer _amountToProject to _project from lender account
-       _currency.safeTransferFrom(_msgSender(), _project, _amountToProject);

Affected source code:

• Community.sol#L442-L446

13. Lack of event index

Community use this event so it's under scope

It would be convenient to add an index per account to the ApproveHash event.

event ApproveHash(bytes32 _hash, address indexed _signer);

Affected source code:

• ICommunity.sol#L111

14. Avoid magic numbers

It is not good practice to use magic numbers when solidity literals can be used.

        uint256 _noOfDays = (block.timestamp -
+           _communityProject.lastTimestamp) / 1 days;
-           _communityProject.lastTimestamp) / 86400; // 24*60*60

Affected source code:

• Community.sol#L686

15. Ensure formula

Although solidity does it well, it is more readable to use '('

        /// Interest formula = (principal * APR * days) / (365 * 1000)
        // prettier-ignore
        uint256 _unclaimedInterest = 
-               _lentAmount *
+               (_lentAmount *
                _communities[_communityID].projectDetails[_project].apr *
+               _noOfDays) /
-               _noOfDays /
                365000;

Affected source code:

• Community.sol#L688-L694

• Gas
  • 1. Is DebtToken needed?
  • 2. Use Custom Errors instead of Revert Strings to save Gas
  • 3. Remove None
  • 4. ++i costs less gas compared to i++ or i += 1
  • 5. There's no need to set default values for variables
  • 6. Don't use storage variables for loops condition
  • 7. Improve deserialization
  • 8. Reorder conditionals to be cheaper
  • 9. Save storage access
  • 10. Reduce code size
  • 11. Improve logic
  • 12. Use memory instead storage

Gas

1. Is DebtToken needed?

It is possible that the DebtToken is not necessary, its functionality is to keep track of the amount of debt that an address has, but this debt is not linked to the projects, so either a DebtToken contract is created per project, or this amount is a sum of all the debts of the account. It is possible that the project can be redesigned to only have a debt mapping and thus eliminate this contract, with the consequent gas savings and also improving debt traceability.

Affected source code:

• DebtToken.sol#L11

2. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In this case, the statements are short, so it is recommended to use custom errors.

Affected source code:

• DebtToken.sol#L31-L34
• DebtToken.sol#L50
• ProjectFactory.sol#L36
• ProjectFactory.sol#L64-L67
• ProjectFactory.sol#L84
• Tasks.sol#L44
• Tasks.sol#L50
• Tasks.sol#L56-L59
• Tasks.sol#L124
• HomeFiProxy.sol#L41
• HomeFiProxy.sol#L81
• HomeFiProxy.sol#L105-L108
• HomeFiProxy.sol#L133
• Disputes.sol#L39
• Disputes.sol#L46
• Disputes.sol#L52
• Disputes.sol#L61-L65
• Disputes.sol#L106-L109
• Disputes.sol#L183
• HomeFi.sol#L73
• HomeFi.sol#L78
• HomeFi.sol#L84
• HomeFi.sol#L142
• HomeFi.sol#L191
• HomeFi.sol#L255-L260
• Project.sol#L123
• Project.sol#L132
• Project.sol#L135
• Project.sol#L150-L153
• Project.sol#L176
• Project.sol#L189-L202
• Project.sol#L238-L245
• Project.sol#L277
• Project.sol#L301-L304
• Project.sol#L308
• Project.sol#L341
• Project.sol#L369
• Project.sol#L406
• Project.sol#L511
• Project.sol#L515-L530
• Project.sol#L753
• Project.sol#L906
• Community.sol#L69
• Community.sol#L75
• Community.sol#L81
• Community.sol#L90
• Community.sol#L131
• Community.sol#L159
• Community.sol#L191
• Community.sol#L235
• Community.sol#L241
• Community.sol#L248-L251
• Community.sol#L312
• Community.sol#L347
• Community.sol#L353
• Community.sol#L384
• Community.sol#L400
• Community.sol#L491
• Community.sol#L536-L539
• Community.sol#L557
• Community.sol#L568
• Community.sol#L764
• Community.sol#L792

3. Remove None

According to the documentation The status None is not defined:

Task	Entity defined by a cost, a subcontractor, and a status. a task can be **Inactive, Active or Complete**. Tasks can be flagged as Allocated when budget for this task has bee provisioned. When subcontractor confirms the assignment on a task it is being flagged as SCConfirmed

if we use Inactive in the first position, or we remove None like this:

enum TaskStatus {
-   None,
    Inactive,
    Active,
    Complete
}

function initialize(Task storage _self, uint256 _cost) public {
    _self.cost = _cost;
-   _self.state = TaskStatus.Inactive;
    _self.alerts[uint256(Lifecycle.None)] = true;
}

We can avoid to set the Inactive state in every initialize calls.

Affected source code:

• Tasks.sol#L24-L25

4. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• Tasks.sol#L181
• HomeFiProxy.sol#L87
• HomeFiProxy.sol#L136
• Project.sol#L248
• Project.sol#L250
• Project.sol#L311
• Project.sol#L322
• Project.sol#L368
• Project.sol#L603
• Project.sol#L625
• Project.sol#L650
• Project.sol#L672
• Project.sol#L710
• Community.sol#L624

5. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• Tasks.sol#L181
• HomeFiProxy.sol#L87
• HomeFiProxy.sol#L136
• Project.sol#L248
• Project.sol#L311
• Project.sol#L322
• Project.sol#L412
• Community.sol#L624

6. Don't use storage variables for loops condition

It's cheaper to cache the storage variable inside a local variable and iterate over it.

Affected source code:

• Community.sol#L624

7. Improve deserialization

It's possible to save a lot of logic (and gas) if decode the _actionType as an enum.

        // Decode params from _data
        (
            address _project,
            uint256 _taskID,
-           uint8 _actionType,
+           ActionType _actionType,
            bytes memory _actionData,
            bytes memory _reason
-       ) = abi.decode(_data, (address, uint256, uint8, bytes, bytes));
+       ) = abi.decode(_data, (address, uint256, ActionType, bytes, bytes));

-       // Revert if _actionType is invalid
-       require(
-           _actionType > 0 && _actionType <= uint8(ActionType.TaskPay),
-           "Disputes::!ActionType"
-       );

        // Store dispute details
        Dispute storage _dispute = disputes[disputeCount];
        _dispute.status = Status.Active;
        _dispute.project = _project;
        _dispute.taskID = _taskID;
        _dispute.raisedBy = _signer;
-       _dispute.actionType = ActionType(_actionType);
+       _dispute.actionType = _actionType;
        _dispute.actionData = _actionData;

        // Increment dispute counter and emit event
        emit DisputeRaised(disputeCount++, _reason);

Affected source code:

• Disputes.sol#L96-L121

8. Reorder conditionals to be cheaper

It's better to check first the internal checks, and after it, do the external calls.

-       bool _result = _projectInstance.builder() == _address ||
+       bool _result = _sc == _address ||
            _projectInstance.contractor() == _address ||
+           _projectInstance.builder() == _address;
-           _sc == _address;
        require(_result, "Disputes::!Member");

Affected source code:

• Disputes.sol#L180-L183

9. Save storage access

The method mintNFT returns the projectCount storage value as memory variable, so it's better to reuse this value instead of call the storage again.

-       mintNFT(_sender, string(_hash));
        // Update project related mappings
        projects[projectCount] = _project;
-       projectTokenId[_project] = projectCount;
+       projectTokenId[_project] = mintNFT(_sender, string(_hash));

Affected source code:

• HomeFi.sol#L225-L229

10. Reduce code size

Changing the order of the signatures it's possible to reduce the logic and the contract size.

+           checkSignatureValidity(contractor, _hash, _signature, 0);
-           if (contractorDelegated) {
+           if (!contractorDelegated) {
+               checkSignatureValidity(builder, _hash, _signature, 1);
-               //  Check contractor's signature
-               checkSignatureValidity(contractor, _hash, _signature, 0);
            }
-           // When builder has not delegated rights to contractor
-           else {
-               // Check for both B and GC signatures
-               checkSignatureValidity(builder, _hash, _signature, 0);
-               checkSignatureValidity(contractor, _hash, _signature, 1);
-           }

Affected source code:

• Project.sol#L805-L815

+           checkSignatureValidity(contractor, _hash, _signature, 0);
+           checkSignatureValidity(_sc, _hash, _signature, 1);
-           if (contractorDelegated) {
+           if (!contractorDelegated) {
+               checkSignatureValidity(builder, _hash, _signature, 2);
-               // Check for GC and SC sign
-               checkSignatureValidity(contractor, _hash, _signature, 0);
-               checkSignatureValidity(_sc, _hash, _signature, 1);
            }
-           // When builder has not delegated rights to contractor
-           else {
-               // Check for B, SC and GC signatures
-               checkSignatureValidity(builder, _hash, _signature, 0);
-               checkSignatureValidity(contractor, _hash, _signature, 1);
-               checkSignatureValidity(_sc, _hash, _signature, 2);
-           }

Affected source code:

• Project.sol#L850-L861

11. Improve logic

It doesn't need to delete or check the signature always. It's better to do the logic according that what happened.

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
+       if (approvedHashes[_address][_hash]) {
+           delete approvedHashes[_address][_hash];
+           return;
+       }
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
+       if (_recoveredSignature == address(0) || _recoveredSignature != _address) revert("Project::invalid signature");
-       require(
-           _recoveredSignature == _address || approvedHashes[_address][_hash],
-           "Project::invalid signature"
-       );
-       // delete from approvedHash
-       delete approvedHashes[_address][_hash];
    }

Affected source code:

• Project.sol#L875-L892

    function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal virtual {
+       if (approvedHashes[_address][_hash]) {
+           delete approvedHashes[_address][_hash];
+           return;
+       }
        // Decode signer
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );

        // Revert if decoded signer does not match expected address
-       // Or if hash is not approved by the expected address.
+       if (_recoveredSignature == address(0) || _recoveredSignature != _address) revert("Community::invalid signature");
-       require(
-           _recoveredSignature == _address || approvedHashes[_address][_hash],
-           "Community::invalid signature"
-       );
- 
-       // Delete from approvedHash. So that signature cannot be reused.
-       delete approvedHashes[_address][_hash];
    }

Affected source code:

• Community.sol#L871-L893

It's not needed to emit the '0x' value, and it's cheaper to emit an empty one.

Affected source code:

-       _reduceDebt(_communityID, _project, _repayAmount, "0x");
+       _reduceDebt(_communityID, _project, _repayAmount, "");

• Community.sol#L468

12. Use memory instead storage

Since all the values of the ProjectDetails struct are returned, it is cheaper to avoid continuous storage accesses and use memory.

    function projectDetails(uint256 _communityID, address _project)
        external
        view
        virtual
        override
        returns (
            uint256,
            uint256,
            uint256,
            uint256,
            bool,
            uint256,
            uint256,
            uint256
        )
    {
+       ProjectDetails memory _communityProject = _communities[_communityID]
-       ProjectDetails storage _communityProject = _communities[_communityID]
            .projectDetails[_project];

        return (
            _communityProject.apr,
            _communityProject.lendingNeeded,
            _communityProject.totalLent,
            _communityProject.publishFee,
            _communityProject.publishFeePaid,
            _communityProject.lentAmount,
            _communityProject.interest,
            _communityProject.lastTimestamp
        );
    }

Affected source code:

• Community.sol#L648