Rigor Protocol contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L115 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L194

Vulnerability details

Impact

The contract doesn't verify that the lenderFee doesn't exceed 100%. If that is the case, it will trigger an underflow in the Community contract. That affects the availability of the protocol.

The admin controls the variable's value, but mistakes can happen. Adding the necessary safe guards in your contract is a better form of protection.

Proof of Concept

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L397

If the fee is over 100%, the fee will be bigger than the lending amount causing an underflow. Currently, the fee logic uses the wrong formula but that's another issue. If the correct formula is used, this issue is valid.

Tools Used

none

Recommended Mitigation Steps

Add a reasonable limit for the fee, e.g. 5%

Report

• Low
  • L-01: use a two-step process for critical address changes
  • L-02: Disputes contract doesn't initialize dependencies
  • L-03: tokenCurrency variables inside Community contract are unused
  • L-04: Protocol doesn't support short signatures (EIP-2098)
  • L-05: SignatureDecoder allows signature malleability
  • L-06: No possibility of modifying HomeFi's currencies
  • L-07: No possibility of disabling a contract
  • L-08: HomeFiProxy.getLatestAddress() doesn't follow the comment's specs

Low

L-01: use a two-step process for critical address changes

Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.

Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L157

L-02: Disputes contract doesn't initialize dependencies

Inside its initialize() function it should also initialize ERC2771ContextUpgradeable and ReentrancyGuardUpgradeable.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Disputes.sol#L80

L-03: tokenCurrency variables inside Community contract are unused

All three tokenCurrency variables are unused. This could point to a faulty implementation although I think that the call to HomeFi is correct. I'd advise deleting them.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L33-L35

L-04: Protocol doesn't support short signatures (EIP-2098)

The compact signature representation (EIP-2098) is a more efficient way to handle signatures. Your protocol doesn't support them because it only allows signatures that have a length of 65. The short version only has a length of 64.

OpenZeppelin's library supports them: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L58-L87

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/libraries/SignatureDecoder.sol#L25

L-05: SignatureDecoder allows signature malleability

An attacker can modify the signature slightly to get another valid signature for the content. I couldn't find any relevant attack vector for this issue. But, you might introduce one in one of your upgrades. I'd advise fixing the issue now.

You can find a solution for it in the OZ library: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L154-L165

A more detailed explanation: https://medium.com/immunefi/intro-to-cryptography-and-signatures-in-ethereum-2025b6a4a33d

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/libraries/SignatureDecoder.sol#L20-L41

L-06: No possibility of modifying HomeFi's currencies

After the initialization, it's not possible to modify the currencies the protocol uses. All the other options can be configured. Only the currencies are immutable. In our space, this might cause you some headaches. I think the design of having three hardcoded currencies is not the best choice here.

A mapping where you can a dynamic amount of valid currencies seems to be a better solution. There seems to be no clear reason why there should only be three currencies. This way you could also disable and reenable currencies on the fly.

Would also save some gas in your validCurrency() function since you can check the mapping instead of each currency individually.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L116-L118

L-07: No possibility of disabling a contract

The HomeFiProxy contract only allows the admin to add a new contract. But, it can't deactivate deprecated ones. This makes the concept of "active" contracts obsolete. Every contract that was created through the HomeFi contract is always active. So you can get all the active contracts by calling getLatestAddress() using the values from allContractNames.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFiProxy.sol#L169

L-08: HomeFiProxy.getLatestAddress() doesn't follow the comment's specs

The function is supposed to return a specific contract's current implementation address. But, it just returns the contract's proxy address. That address is always the same. It doesn't change. To get the implementation contract, you have to call the proxy's function.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFiProxy.sol#L182

Gas Report

• G-01: use ++1 when incrementing a variable
• G-02: use unchecked when incrementing the iterator of a loop
• G-03: cache storage variables in memory to save gas
• G-04: unnecessary nonZero() check in HomeFiProxy

G-01: use ++1 when incrementing a variable

++1 is the cheapest way to increment.

./contracts/mock/HomeFiMock.sol:203:        projectCount += 1;
./contracts/HomeFi.sol:297:        projectCount += 1;
./contracts/Project.sol:179:        hashChangeNonce += 1;
./contracts/Project.sol:250:            _taskCount += 1;
./contracts/Project.sol:290:        hashChangeNonce += 1;
./contracts/libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
./contracts/Community.sol:627:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
./contracts/Project.sol:248:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:311:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:322:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:603:            for (; i < _changeOrderedTask.length; i++) {
./contracts/HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
./contracts/HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) {

G-02: use unchecked when incrementing the iterator of a loop

Realistacally the iterator can't overflow because of the loop's condition. Because of that, you can generally use the unchecked block to save some gas:

for (uint i; i < length;) {

    unchecked {
        ++i;
    }
}

./contracts/libraries/Tasks.sol:181:        for (uint256 i = 0; i < _length; i++) _alerts[i] = _self.alerts[i];
./contracts/Community.sol:627:        for (uint256 i = 0; i < _communities[_communityID].memberCount; i++) {
./contracts/Project.sol:248:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:311:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:322:        for (uint256 i = 0; i < _length; i++) {
./contracts/Project.sol:603:            for (; i < _changeOrderedTask.length; i++) {
./contracts/HomeFiProxy.sol:87:        for (uint256 i = 0; i < _length; i++) {
./contracts/HomeFiProxy.sol:136:        for (uint256 i = 0; i < _length; i++) {

G-03: cache storage variables in memory to save gas

Instead of reading the same variable from storage multiple times, you should cache it in memory.

e.g.

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFi.sol#L289-L296 (projectCount)

G-04: unnecessary nonZero() check in HomeFiProxy

• https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/HomeFiProxy.sol#L200

You don't have to call the nonZero modifier here because the underlying OZ library already checks for that. It doesn't allow the implementation address to not be a contract.