Rigor Protocol contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Transaction replays on unrelated functions

Proof of Concept

abi.decode has a very high level of ambiguity when decoding abi.encode data. Variable types do not follow many of the conversion rules that apply to regular setting. All abi.decode does is take the bytes given to it and unpacks it and stores is at the requested variables. This means that an encoded uint256 could be unpacked as nearly any other type of data. The output is dictated by the abi.decode call not the abi.encode call. To make matters worse, the length of the input data does not need to be exactly the same as the output for abi.decode.

event BroadcastUint256 (uint256 indexed myUint);
event BroadcastAddress (address indexed myAddress);

function DecodeWrongLengthAndVarType () public {

    uint256 first = uint256(1);
    uint256 second = uint256(2);
    uint256 third = uint256(3);

    bytes memory data = abi.encode(first, second, third);

    (address firstDecoded, uint256 secondDecoded) = abi.decode(data, (address, uint256));

    emit BroadcastAddress(firstDecoded);
    emit BroadcastUint256(secondDecoded);
}

I've included some sample code above, feel free to plug it into remix and test yourself. Let's use the above decode line as an example. On the right side of the function, abi.decode is decoding data and storing it as an address and uint256. As long as data is at least 64 bytes long (address=32 bytes, uint256=32 bytes) then it will successfully store a value for both firstDecoded and secondDecoded. In the above example data is 96 bytes, abi.decode will read the first 64 bytes, store them as firstDecoded and secondDecoded, then skip the last 32 bytes. This introduces a huge amount of data malleability, because input data does not have to match the outputs of abi.decode in either length or type, the only requirement is that the input data length is at least as long as the output. Given how loose the requirements are, data from a majority of functions in Project.sol and Community.sol can be submit interchangeably and the data will be processed by abi.decode without issue.

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

checkSignatureValidity(builder, _hash, _signature, 0);

The other point of concern is the method by which signatures are specified. The issue is that the builder is always uses the 0 index. This means that the signature will be valid for the data for every function it is submit with, due to it always being signed with the same signatureIndex.

When a transaction is mined or even added the mempool, all the input data becomes public. This would give malicious actors access to all data and signatures. It would also allow them to find and test every possible replay opportunity. It isn't just limited to project.sol because community.sol uses the same signature/data scheme and a majority of functions that use this pattern can be called by anyone. It's clear that Rigor is a very ambitious product and will continue to expand to become more feature rich. With this expansion will come an even larger number of functions and signed data, which will continually widen the opening for data replay.

This is open ended and there are very large number of potential vectors and combinations to consider. I was not able to find a specific example but given the scope I think it is worth including, so I am submitting this as medium risk.

Tools Used

Recommended Mitigation Steps

I strongly recommend implementing a stricter form of permissioning. I believe to fully remove the risk of replay, a majority of functions should be permissioned to the appropriate builder/contractor/subcontractor though the use of modifiers.

Lines of code

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

Vulnerability details

Impact

Builder reduces their interest by half

Proof of Concept

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

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

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

 _communityProject.lastTimestamp = block.timestamp;

In Community.sol#returnToLender, the number of days for accumulating interest is rounded down to the nearest day. Then in Community.sol#claimInterest _communityProject.lastTimestamp is updated to block.timestamp. If a builder were to make a small repayment every 1.99 days, only 1 day worth of interest would accumulate every ~2 days. This would allow them to reduce their total paid interest by half.

This vulnerability cannot be used to avoid interest completely because if the accumulated interest == 0 then Community.sol#claimInterest won't update the timestamp.

Tools Used

Recommended Mitigation Steps

Change L847 to:

_communityProject.lastTimestamp = (block.timestamp / 86400) * 86400;

Lines of code

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

Vulnerability details

Impact

All projects created while fee > 1000 can never receive funding

Proof of Concept

HomeFi.sol#replaceLenderFee allows fees > 1000 (100%). If set > 1000 all projects created after would never be able to receive funds due to lenderFee causing an underflow error in Community.sol#lendToProject.

Tools Used

Recommended Mitigation Steps

Implement a max fee:

require(lenderFee >= 30);

Lines of code

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

Vulnerability details

Impact

Builder forced to pay higher interest

Proof of Concept

Contract is pausable and functions that reduce debt do not function when contract is paused. Interest continues to accrue during that period of time, forcing the builder to pay more interest because they are unable to repay while it's paused.

Tools Used

Recommended Mitigation Steps

Remove whenNotPaused modifer from functions that reduce user debt.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L501-L506 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L108-L115

Vulnerability details

Impact

User is unable to revoke previously approved hash

Proof of Concept

If user reconsiders or notices something malicious about the hash after signing, they should be able to revoke the hash. For example the user approves a hash only to find out later that the hash has been spoofed and they weren't approving what they thought they were. To protect themselves the user should be able to revoke approval, otherwise it may lead to loss of funds or access.

Tools Used

Recommended Mitigation Steps

Add the following function:

function revokeHash(bytes32 _hash) external virtual {
    approvedHashes[_msgSender()][_hash] = false;
}

QA Report

Low Risk Findings

[L-01] HomeFi.sol implements a secondary non-EIP1967 admin address

Description

The underlying proxy for HomeFi.sol stores the admin address in the EIP1967 compliant location at:

bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)

HomeFi.sol implements a secondary non-EIP1967 admin address in L52, which is set in L113 of initialize.

Lines of Code

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

Mitigation

admin should be renamed to something else such as owner to avoid any confusion

[L-02] Proxies implement a payable fallback and receive

Description

Contracts never deal directly with eth, but implement a payable fallback and receive

Lines of Code

N/A

Mitigation

Fallback should not be payable and receive should revert.

[L-03] HomeFi.sol mints users an NFT with no function when they call createProject

Description

HomeFi.sol mints an NFT when calling createProject, but never uses any feature associated with it. It consumes a lot of gas to mint an NFT then never use it for anything. Implementing the ERC721 standard increases gas cost to deploy the contract because of the extra code required.

Lines of Code

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

Mitigation

Don't mint NFTs when calling createProject and don't implement ERC721

[L-04] Current method for storing acceptable currency is limited

Description

Current method of storing acceptable currency can only ever support 3 currencies. In order to add more, a new implementation would have to be deployed. If currencies were stored in a mapping it would be easy and convenient to add new currencies without having to deploy a new contract.

Lines of Code

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

Mitigation

Store the approved currencies in a mapping(address => bool) to approve a currency set the bool for that currency address to true. To validate a currency simply check if the mapping returns true for its address.

Non-Critical Findings

[NC-01] Community.sol has no way to remove members

Description

There is no way to remove a member from a community after they've been added. It could help with bloat if there was a way to remove users.

Lines of Code

N/A

Mitigation

Add a function that allows the owner to remove users. Additionally add a nonce to addMember so the transaction data can't be replayed after the user is removed