Rigor Protocol contest - sseefried's results

Lines of code

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

Vulnerability details

Impact

A builder who only wishes to pay interest only (right up until the point they sell the house and can repay the loan) can call repayLender approximately, but not quite, every two days but only pay one day's worth of interest each time. This is because the interest is calculated on a whole number of days, not fractions of days.

The impact is that the lender gets less money than they otherwise would have.

Proof of Concept

• assume a builder that wants to hang on to their money as long as possible but reduce the amount of interest they have to pay. Thus they only want to pay interest until the final payment.
• the builder calls repayLender with 1 day's worth of interest but close to the 2 day boundary. e.g. 2 days - 5 minutes
• the chain of nested functions calls is repayLender -> _reduceDebt -> claimInterest -> returnToLender
• the chain of function calls eventually leads to returnToLender
• On line 685-686 _noOfDays is calculated as 1 even though block.timestamp - _communityProject.lastTimeStamp is approximately 2 days.
• _unclaimedInterest is calculated to be 1 day's worth of interest
• returnToLender finishes executing and we return to claimInterest
• since _interestedEarned > 0 on line 840 then _communityProject.lastTimestamp is updated to block.timestamp.
• builder repeats this process until they sell the house and can repay the principle of the loan
• since they call repayLender approximately every (but not quite) two days -- but they always pay only one day's worth of interest -- they only end up paying half the interest they otherwise would have

This attack can be carried out at differing time intervals but for less and less effect. Paying interest (nearly) every 3 days gets means the attacker pays $2\over{3}$ of the expected interest, (nearly) every 4 days means $3\over{4}$ of the expected interest, etc.

Recommended Mitigation Steps

Interest should not be calculated in whole days, but instead at the level of whole seconds, since it would be infeasible to perform the exploit with that periodicity.

However, you might still want to make the minimum period between calls to repayLender at least one day.

Here is a sample implementation for the relevant section of the repayLender function.

require(block.timestamp - _communityProject.lastTimestamp >= 1 days, "Called too soon");
// Calculate number of days difference current and last timestamp
uint256 _noOfSeconds = (block.timestamp -
    _communityProject.lastTimestamp);

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

Lines of code

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

Vulnerability details

Impact

Since there is no nonce in the data decoded at the beginning of function escrow, a builder can call the function multiple times reducing their debt as much as they wish.

Proof of Concept

• A builder has a debt of $50,000
• A lender, a builder, and an escrow agent all enter a bar sign a message that will reduce the debt of the builder by $5,000, upon receipt of physical cash.
• Function escrow is called and debt is reduced to $45,000.
• The builder, using the same _data and _signature then calls escrow a further 9 times reducing their debt to zero.

Recommended Mitigation Steps

1. Similar to function publishProject, add a new field into the ProjectDetails struct called escrowNonce.

2. Modify function escrow to check this nonce and update it after the debt has been reduced.

See the diff below for full changes.

diff --git a/contracts/Community.sol b/contracts/Community.sol
index 1585670..b834d0e 100644
--- a/contracts/Community.sol
+++ b/contracts/Community.sol
@@ -15,7 +15,7 @@ import {SignatureDecoder} from "./libraries/SignatureDecoder.sol";

 /**
  * @title Community Contract for HomeFi v2.5.0
-
+
  * @notice Module for coordinating lending groups on HomeFi protocol
  */
 contract Community is
@@ -520,10 +520,11 @@ contract Community is
             address _agent,
             address _project,
             uint256 _repayAmount,
+            uint256 _escrowNonce,
             bytes memory _details
         ) = abi.decode(
                 _data,
-                (uint256, address, address, address, address, uint256, bytes)
+                (uint256, address, address, address, address, uint256, uint256, bytes)
             );

         // Compute hash from bytes
@@ -540,6 +541,12 @@ contract Community is
             _lender == _communities[_communityID].owner,
             "Community::!Owner"
         );
+        ProjectDetails storage _communityProject =
+          _communities[_communityID].projectDetails[_project];
+        require(
+            _escrowNonce == _communityProject.escrowNonce,
+            "Community::invalid escrowNonce"
+        );

         // check signatures
         checkSignatureValidity(_lender, _hash, _signature, 0); // must be lender
@@ -548,6 +555,7 @@ contract Community is

         // Internal call to reduce debt
         _reduceDebt(_communityID, _project, _repayAmount, _details);
+        _communityProject.escrowNonce = _communityProject.escrowNonce + 1;
         emit DebtReducedByEscrow(_agent);
     }

diff --git a/contracts/interfaces/ICommunity.sol b/contracts/interfaces/ICommunity.sol
index c45bbf0..652f51c 100644
--- a/contracts/interfaces/ICommunity.sol
+++ b/contracts/interfaces/ICommunity.sol
@@ -29,6 +29,7 @@ interface ICommunity {
         uint256 lentAmount; // current principal lent to project (needs to be repaid by project's builder)
         uint256 interest; // total accrued interest on `lentAmount`
         uint256 lastTimestamp; // timestamp when last lending / repayment was made
+        uint256 escrowNonce; // signing nonce to use when reducing debt by escrow
     }

Lines of code

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

Vulnerability details

Impact

There are no nonces as part of the data for updating tasks in function changeOrder. It is possible that a builder and sub-contractor might agree to a price increase, followed by another price increase later by signing appropriate messages. However, the builder can replay the original message and signature reducing the price down to the first price which is lower than the final price.

Proof of Concept

• Assume builder has no contractor but does have sub-contractors
• Builder and sub-contractor signed message to change task 1 cost from $500 to $600
• Later, when they realise things are going to cost more, the builder and sub-contractor sign a new message to change task 1 cost from $600 to $1000.
• Builder replays the first signed message to reduce the cost of task 1 back down to $600
• when setComplete is eventually called the sub-contractor only gets $600 even though a new agreement existed for $1000.

Recommended Mitigation Steps

Add a nonce field to the Task struct and make the nonce part of the changeOrder function. See below.

diff --git a/contracts/Project.sol b/contracts/Project.sol
index 4a1dcf5..ddb480c 100644
--- a/contracts/Project.sol
+++ b/contracts/Project.sol
@@ -393,8 +393,9 @@ contract Project is
             uint256 _taskID,
             address _newSC,
             uint256 _newCost,
+            uint256 _nonce,
             address _project
-        ) = abi.decode(_data, (uint256, address, uint256, address));
+        ) = abi.decode(_data, (uint256, address, uint256, uint256, address));

         // If the sender is disputes contract, then do not check for signatures.
         if (_msgSender() != disputes) {
@@ -404,6 +405,8 @@ contract Project is

         // Revert if decoded project address does not match this contract. Indicating incorrect _data.
         require(_project == address(this), "Project::!projectAddress");
+        require(_nonce == tasks[_taskID].nonce,"Project::!nonce");
+        tasks[_taskID].nonce = tasks[_taskID].nonce + 1;

diff --git a/contracts/libraries/Tasks.sol b/contracts/libraries/Tasks.sol
index a7b4815..d186beb 100644
--- a/contracts/libraries/Tasks.sol
+++ b/contracts/libraries/Tasks.sol
@@ -13,6 +13,7 @@ struct Task {
     address subcontractor; // Subcontractor of task
     // Lifecycle //
     TaskStatus state; // Status of task
+    uint256 nonce; // Nonce used for changing orders
     mapping(uint256 => bool) alerts; // Alerts of task
 }

Lines of code

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

Vulnerability details

Impact

The admin can set the lenderFee too high either maliciously or by accident.

If the lenderFee > 1000 the impact is that the Community.lendToProject function always reverts due to arithmetic underflow.

If the lenderFee is unreasonably high then lenders will have to pay a lot just for the privilege of being able to give out loans.

Proof of Concept

1. Arithmetic overflow

• Admin accidentally calls replaceLenderFee with value > 1000
• Community owner calls lendToProject
• on lines 393-394 the _lenderFee is calculated to be greater than _lendingAmount
• line 397 causes an arithmetic underflow and a revert

2. High fees

• Malicious admin calls replaceLenderFee with value > 500 (i.e. > 50%)
• Community owner calls lendToProject
• on lines 393-394 the _lenderFee is calculated to be more than half of _lendingAmount
• on line 443 the lender must transfer an unreasonably large fee to the HomeFi treasury.

Recommended Mitigation Steps

The solution is to define a constant that defines the maximum fee rate in the HomeFicontract. Something like

uint256 public immutable MAX_FEE_RATE = 100; // 10%

and then modify replaceLenderFee to be

function replaceLenderFee(uint256 _newLenderFee)
    external
    override
    onlyAdmin
{
    // Revert if no change in lender fee
    require(lenderFee != _newLenderFee, "HomeFi::!Change");
    require(_newLenderFee <= MAX_FEE_RATE, "HomeFi::!FeeRateTooHigh");

    // Reset variables
    lenderFee = _newLenderFee;

    emit LenderFeeReplaced(_newLenderFee);
}

I also recommend making a distinction between the "fee rate" and the "fee" in the variable names. The fee rate is a percentage, whereas the fee is an amount that is paid and is calculated from the fee rate.

Lines of code

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

Vulnerability details

Impact

The EIP2771 standard makes it very clear that trusting your forwarder is very important, so I don't imagine that the HomeFi would knowingly choose an untrustworthy forwarder. However, it still makes sense to place the admin on a slightly higher tier of trust than the forwarder. The admin is the one user that should not be able to be impersonated under any circumstances. Most importantly, no one but the admin should be able to called replaceAdmin.

Unfortunately, the isAdmin modifier of the HomeFi contract allows for just such impersonation. Although unlikely, an untrustworthy forwarder could change the admin, in which case regain of control of the HomeFi contract is impossible.

Although the damage an untrustworthy forwarder could do is very high, the chance that they will be untrustworthy in the first place is low, which is why I have rated this finding as Medium Risk.

Proof of Concept

• Assume an untrustworthy forwarder
• Line 73 is require(admin == _msgSender(), "HomeFi::!Admin");. Technically the forwarder can impersonate the admin by placing the admin address in the last 20 bytes of the EIP2771 message.
• The malicious forwarder can then call replaceAdmin with whoever they want

Recommended Mitigation Steps

At least a couple of the functions should not use _msgSender() -- which allows for trusted forwarder to spoof -- and just use msg.sender.

Change to

modifier onlyAdmin() {
    require(admin == msg.sender, "HomeFi::!Admin");
    _;
}

With this mitigation in place the admin cannot be replaced and they can always call setTrustedForwarder to change the forwarder.

The downside to this mitigation is that admin calls would have to pay their own gas fees but this is a small price to pay for the peace of mind that, were a forwarder proved to be untrustworthy, they could be replaced with one that was.

If gas costs really are a concern, another possibility is to have two modifiers, perhaps one called isDefinitelyAdmin and just protect the replaceAdmin function with that modifier.

QA Report

Coding style improvements

• On line Project.sol:579 change _costToAllocate variable name to _allocationBudget since that better reflects that it's the amount the could be used to allocate tasks.

• unnecessary cast to uint256 on Project.sol:200.

Documentation improvements

• Community.sol:849 "Burn" -> "Mint"

• Project.sol:379 If balance is present then it to the builder. Add word: "transfer" or "send"

L-01: For a given project, don't allow lendToProject to be called less than 1 day since last call.

It is extremely unlikely that a lender would work against their own interests but if they were to repeatedly call lendToProject within 1 day then this would lead to unfavourable interest calculations.

If we assume that lastTimestamp for the project is less than 86400 seconds ago then:

• on line 779 claimInterest would not claim any interest
• if a builder were to call repayLender zero interest would also be calculated when claimInterest was called inside the call.

This is because claimInterest calls returnToLender which will then cause _interestEarned to be set to 0. When _interestEarned is zero the state of _communityProject is not updated.

This situation is extremely unlikely which is why I have classified this as Low Risk. However, to prevent user error perhaps the lendToProject should revert if lastTimestamp is less than 1 day ago.

L-02: No functionality to change contractor

From time to time it might be necessary to fire a contractor or sub-contractor. There does not appear to be any functionality for this.

L-03: No time-locks on admin "setter" functions

Various "setter" functions in HomeFi don't have any time locks enabled on them. Consider using the time-lock pattern for those functions that could introduce an admin rug-pull risk.

N-01: allocateFunds may need to be called after Project.lendToProject

Function allocateFunds can fail to complete as it can only allocate _maxLoop == 50 tasks at a time. The builder needs to be aware that they need to call allocateFunds multiple times until an IncompleteAllocation event is not emitted.

It might be a good idea to have two events that distinguish between the two reasons that events are not allocated.

1. _maxLoop tasks allocated
2. Not enough funds