Rigor Protocol contest - hansfriese's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L685-L694

Vulnerability details

Impact

Currently, it calculates interest using the number of days and builders would pay nearly half or less interest than they should.

So lenders wouldn't get the interest as expected and it means builders can steal the interest from lenders.

Proof of Concept

As we can see here, interest is calculated using _noOfDays so below 2 scenarios would be possible.

1. Scenario 1

• A builder published his project with apr = 5% here.
• A lender(community owner) lent some funds here.
• After that, a builder repaid a dust amount like 1 wei once per 1.99 days using repayLender().
• Then _noOfDays will be 1 and _unclaimedInterest will be an interest of 1 day.
• And _interestEarned will be positive as it's an interest for 1 day. So _communityProject.lastTimestamp will be updated properly.
• So a builder paid an interest of 1 day after 1.99 days are passed, so a lender will receive around 2.5% interest only 1 year later.

2. Scenario 2

• A builder published a project with a high apr like 10% and a lender lent some funds same as scenario 1.
• A builder requests to increase lendingNeeded every day using toggleLendingNeeded().
• As apr is high enough, a lender might lend increased funds every day using lendToProject().
• If the interest was updated within 1 day, the new interest will be zero again as _noOfDays = 0.
• Also, lastTimestamp will be updated properly here.
• So if a lender lends additional funds every day by following a builder's request, a lender wouldn't get any interest.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we should use _noOfSeconds here instead of _noOfDays.

uint256 _noOfSeconds = block.timestamp - _communityProject.lastTimestamp;

uint256 _unclaimedInterest = 
        _lentAmount *
        _communities[_communityID].projectDetails[_project].apr *
        _noOfSeconds / 365 / 86400 / 1000;

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L386

Vulnerability details

Impact

changeOrder() can be used to change subcontractor.

But if a project builder approves the signature by fault or the contractor is delegated, a malicious contractor and subcontractor might reinitialize an already completed task and complete again to receive the task cost twice.

Proof of Concept

Currently changeOrder() doesn't check whether the task is completed or not so the below scenario would be possible.

• A project builder(B), contractor(GC), and subcontractor(SC1) have completed one task(T) and SC1 received a payment.
• Malicious GC, SC1, and SC2 discussed stealing the project cost by completing again.
• They requested to change the subcontractor of T from SC1 to SC2.
• B didn't notice the task was completed already and agreed by fault. If GC is a delegated contract, it's 100% possible as they don't need B's agreement at all.
• Finally, changeOrder() was called properly and the state of project became Inactive here.
• After that, SC2 accepted the invitation and received the cost after completing the task.
• As a result, the task was paid twice and this project wouldn't work properly as funds were paid unexpectedly.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Once the task has been completed, we should prevent changing any properties of the task because the cost was paid already and it's irreversible.

We should add a below require() here.

require(tasks[_taskID].state != TaskStatus.Complete, "Completed already");

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L402

Vulnerability details

Impact

The changeOrder() function will revert when it's called for the tasks that don't have subcontractors.

As a result, the project builder and contractor can't change the cost of a task until the subcontractor is confirmed.

Proof of Concept

The changeOrder() is used to change the cost or subcontractor of a task and there is no documentation that this function must be called only after a subcontractor is confirmed.

Also, it's reasonable to be able to change the cost when a subcontractor isn't confirmed yet.

But when it checks signature here, it assumes the task has a confirmed subcontractor already and checkSignatureTask() will revert in other cases.

So this function will be useless for non SCConfirmed tasks.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should check separately in case the subcontractor is confirmed or not here.

if (getAlerts(_taskID)[2]) {
    // If subcontractor has confirmed.
    checkSignatureTask(_data, _signature, _taskID);
} else {
    // If subcontractor not has confirmed.
    checkSignature(_data, _signature);
}

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L115 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L185

Vulnerability details

Impact

The owner may set the lenderFee as high as he wants and claim almost 100% of the funds.

Proof of Concept

From this comment, lenderFee can be in range [1, 1000].

But there is no requirement when admin sets lenderFee here and here.

So the admin can even set lenderFee = 1000000, then he will take around 1000000/(1000000 + 1000) = 99.9% as a fee.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

It's recommended to set some reasonable upper bounds for the lenderFee like 5%.

We can add below requirements here and here.

require(_lenderFee >= 1, "zero lenderFee");
require(_lenderFee <= 50, "more than 5%");

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L891 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L842-L862

Vulnerability details

Impact

According to the Glossary, "Builder can act as a contractor." and "Contractor can act as a subcontractor.".

But Project.checkSignatureTask() might revert when a builder or contractor is the same as a subcontractor.

Proof of Concept

According to the Glossary, it's possible that the builder or contractor is the same as the subcontractor.

Logically, it's logical also because the builder or contractor would confirm some tasks as a subcontractor and complete by themselves because other subcontractors don't like to manage them as the tasks are too difficult or costs are low.

It would be important for builders to complete all tasks and recover remaining tokens.

Then in checkSignatureTask(), it's possible builder == _sc or contractor == _sc.

And in checkSignatureValidity(), approvedHashes plays an important role to check validity.

Btw the approvedHashes mapping is deleted after the validation and it won't work properly when builder == _sc or contractor == _sc.

So these validations might be failed in this case.

I don't think the builder won't set the same contractor so I don't mention this part.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we don't need to check a signature for _sc when it's the same as builder or contractor.

We can modify checkSignatureTask() like below.

function checkSignatureTask(
    bytes calldata _data,
    bytes calldata _signature,
    uint256 _taskID
) internal {
    // Calculate hash from bytes
    bytes32 _hash = keccak256(_data);

    // Local instance of subcontractor. To save gas.
    address _sc = tasks[_taskID].subcontractor;

    // When there is no contractor
    if (contractor == address(0)) {
        // Just check for B and SC sign
        checkSignatureValidity(builder, _hash, _signature, 0);

        if(_sc != builder) {
            checkSignatureValidity(_sc, _hash, _signature, 1);
        }
    }
    // When there is a contractor
    else {
        // When builder has delegated his rights to contractor
        if (contractorDelegated) {
            // Check for GC and SC sign
            checkSignatureValidity(contractor, _hash, _signature, 0);

            if(_sc != contractor) {
                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);

            if(_sc != builder && _sc != contractor) {
                checkSignatureValidity(_sc, _hash, _signature, 2);
            }
        }
    }
}

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L455 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L484 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L509

Vulnerability details

Impact

Builders can't repay when the system is paused so they must pay more interest for the paused period.

Proof of Concept

Builders can repay to lenders using 3 functions, repayLender(), reduceDebt(), and escrow().

But they all don't work when the system is paused and builders have no way to avoid it.

Furthermore, the HomeFi admin is the main lender of builders and there is no assurance that the admin would pause the community for a while to get more interest.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend thinking of an approach to make 3 repay functions work for paused or modify the interest calculation formula not to add interest for the paused period.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L206

Vulnerability details

Impact

According to the document, "Note that you cannot submit a project with no total budget. Therefore it requires at least one task with a budget > 0.".

But publishProject() doesn't have such a requirement.

Proof of Concept

If a builder publishes his project without any tasks, he can't increase lendingNeeded because of this condition and projectCost() = 0.

So he can't request anything even after he paid the publishFee.

Also, such a project is needless for the community owner as he can't lend funds and it will just increase the memory to save the useless project.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend adding an additional require here like the document.

require(_projectInstance.projectCost() > 0, "cost = 0");

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330

Vulnerability details

Impact

setComplete() function might be called successfully using the past signature when it shouldn't work.

As a result, a task might be completed when a builder doesn't want it.

Proof of Concept

approveHash() function can set only true so there is no method to cancel already approved hash without passing validation here.

So the below scenario would be possible.

• A builder, GC, and SC started a task and SC finished the task.
• They are approved to complete the task and signed the signature.
• But right before to call setComplete() using the signature, the SC felt the cost is too low and raised a dispute to change the order using raiseDispute().
• As I suggested with another medium issue, the task can't be completed when there is an ongoing dispute from this document - "If there is no ongoing dispute about that project, task status is updated and payment is made.". So setComplete() might revert.
• Even if it doesn't check active disputes as now, setComplete() might revert when the funds haven't been allocated and a builder signed by fault.
• After that, the HomeFi admin accepted the dispute, and the cost of the task was increased as SC wanted.
• Then the builder would hope to get more results (or scores) from this task as the cost is increased rather than completed right away.
• But SC can call setComplete() using the previous signature and complete the task without additional work.
• A builder might know about that before and try to update task hash but it will revert because SC doesn't agree to updateTaskHash().
• In this case, it's logical to cancel the approved hash here but there is no such option.

I don't know if there would be similar problems with other functions that use signature and I think it would reduce the risk a little if we add an option to cancel the approved hash.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend modifying approveHash() like below.

function approveHash(bytes32 _hash, bool _bool) external override { //++++++++++++++++++++
    address _sender = _msgSender();
    // Allowing anyone to sign, as its hard to add restrictions here.
    // Store _hash as signed for sender.
    approvedHashes[_sender][_hash] = _bool; //+++++++++++++++++++

    emit ApproveHash(_hash, _sender, _bool); //++++++++++++++++++++++
}

I am not so sure that a similar scenario would be possible in the Community contract also and recommend to change both functions together.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L238

Vulnerability details

Impact

addTasks() function checks this require() to make sure _taskCount is correct.

But it might revert when this function is called after a dispute because it takes a certain time to resolve disputes and other tasks might be added meanwhile.

Proof of Concept

The below scenario would be possible.

• A project contains 10 active tasks(taskCount = 10) and a builder and contractor are going to add one more task.
• There were some disagreements between a builder and contractor so they raised a dispute with _taskCount = 10 using raiseDispute().
• Normally it would take a certain time(like 1 day or more) to resolve the dispute as it must be done by HomeFi owner.
• Meanwhile, if the builder and contractor need to add another task, they should set _taskCount = 10 and taskCount will be 11 after addition here.
• After that, the HomeFi admin agreed to add a task with _taskCount = 10, but it will revert here.

So currently, the project builder and contractor shouldn't add new tasks to make their previous dispute valid.

I think it's reasonable to modify that they can add other tasks even though there is an active dispute.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

I think we can modify not to compare taskCount when it's called from disputes contract.

So we can modify this part like below.

if (_msgSender() != disputes) {
    require(_taskCount == taskCount, "Project::!taskCount");
}
else {
    _taskCount = taskCount;
}

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/Tasks.sol#L160-L164

Vulnerability details

Impact

Tasks.unApprove() doesn't reset the past subcontractor after unapprove so some unexpected result might happen.

Proof of Concept

unApprove() function is used in Project.changeOrder() and such scenario is possible.

• There is a task that funds are allocated and subcontractor(SC) is confirmed.
• They(builder, GC, SC) discuss increasing the cost and call changeOrder() with higher cost and same SC.
• As totalLent is not enough, the task is unapproved here.
• And this condition will be false as a subcontractor wasn't removed during unapprove here.
• So there won't be a new invitation for the same SC and SC wouldn't know he's unconfirmed from the task.
• It will work properly if SC accepts the subcontractor again but SC won't know he must accept again because he only accepted to change the cost and there are no events for the invitation.

Originally, it's not logical to remain a subcontractor after unapprove because the old subcontractor can be confirmed again if he wants.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend removing subcontractor in unApprove().

function unApprove(Task storage _self) internal {
    // State/ lifecycle //
    _self.alerts[uint256(Lifecycle.SCConfirmed)] = false;
    _self.state = TaskStatus.Inactive;
    _self.subcontractor = address(0); //+++++++++++++++++++++++++++++++++++
}

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L903 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L253 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L417

Vulnerability details

Impact

The task of zero cost is useless and there would be no subcontractors to accept such task as no payment after finish.

So if such task is added by mistake, it would require more time and effort to finish because builder must complete it by himself to recover tokens.

Proof of Concept

It will work properly when _amount = 0.

function checkPrecision(uint256 _amount) internal pure { 
    // Divide and multiply amount with 1000 should be equal to amount.
    // This ensures the amount is not too precise.
    require(
        ((_amount / 1000) * 1000) == _amount,
        "Project::Precision>=1000"
    );
}

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend changing like below.

function checkPrecision(uint256 _amount) internal pure { 
    require(_amount > 0, "zero amount");

    // Divide and multiply amount with 1000 should be equal to amount.
    // This ensures the amount is not too precise.
    require(
        ((_amount / 1000) * 1000) == _amount,
        "Project::Precision>=1000"
    );
}