Rigor Protocol contest - indijanc's results

Lines of code

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

Vulnerability details

Impact

Anyone can add himself as a member of community for any future community. This can be done due to a combination of facts:

• Non initialized address storage values are defaulted to address(0)
• addMember doesn't check if community is already created
• checkSignatureValidity doesn't check for address(0)

The same issue exists in publishProject() but didn't verify with a test due to lack of time. The impact here is that a malicious builder could publish a project to a future community, again without the community owner approval or signature. He can add himself as a member beforehand with the above mentioned addMember() flaw.

The same issue may exists in escrow() but couldn't verify and investigate further here due to lack of time.

This would normally seem a critical issue but marked it as medium as I could find a clear way to abuse the broken state in a very harmful way. Being a member of a community doesn't seem to give immediate special access and publishing a project to a community also won't outright give access to funds... Nevertheless I may have missed more implications here due to lack of time and this "backdoor" certainly isn't intended for the Community contract, so I would advise to revise and fix this flaw.

Proof of Concept

This can be verified with a simple test in communityTest.ts:

diff --git a/test/utils/communityTests.ts b/test/utils/communityTests.ts
index 51dc09e..af96801 100644
--- a/test/utils/communityTests.ts
+++ b/test/utils/communityTests.ts
@@ -442,6 +442,32 @@ export const communityTests = async ({

       await expect(tx).to.be.revertedWith('Community::Member Exists');
     });
+    it('Should revert if add member to future community', async () => {
+      const data = {
+        types: ['uint256', 'address', 'bytes'],
+        // signers[10] is malicious
+        // Community ID 10 is a future community, not yet created
+        values: [10, signers[10].address, sampleHash]
+      }
+      const [encodedData, signature] = await multisig(data, [
+        signers[10],
+        signers[10],
+      ]);
+      // malicious user approves msg hash
+      let encodedMsgHash = ethers.utils.keccak256(encodedData);
+      const tx = await communityContract
+        .connect(signers[10])
+        .approveHash(encodedMsgHash);
+      await tx.wait();
+
+      // Mess up the signature length to force 0 address from SignatureDecoder.recoverKey
+      const addMemberTx = communityContract.addMember(encodedData, signature.concat('aa'));
+
+      // Should be reverted for at least one of the following reasons:
+      // - communigy not yet created
+      // - community owner did not sign or apporve msg hash
+      await expect(addMemberTx).to.be.reverted;
+    });
   });

Tools Used

Visual Studio Code

Recommended Mitigation Steps

This could be resolved in addMember() by checking if community of the specified community ID is already created (community.memberCount > 0) but maybe a better and more general resolution for all vulnerable functions is to check for address(0) on input of checkSignatureValidity(address _address, ...).

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L219 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L655 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L807

Vulnerability details

Impact

A malicious delegated contractor can add a huge number of tasks (or one task with a huge cost). This would then pose problems in allocateFunds() as tasks could not be funded. Builder could remove delegation for the contractor but couldn't replace the contractor and so couldn't remove the malicious contractor. The contractor is required to sign various state changes in Project.sol. A delegated contractor can also for example complete tasks which results in transferring funds to subcontractors.

This sounds very problematic and would be critical, but reading through the documentation and the code, I'm assuming there is certain trust incorporated and required for the system to work. Hence I'm assuming the system considers a delegated contractor is trustworthy as is the builder. So while the impact may be big I consider the likelihood quite small.

Proof of Concept

When a contractor is delegated, various operations only need his signature. Project.sol L807

Tools Used

Visual Studio Code

Recommended Mitigation Steps

There's a couple of improvements you could consider:

1. Create a function to update lastAllocatedTask. This could be restricted to Disputes contract or the builder. This could be used against maliciously inserted tasks.
2. Add functionality for Disputes contract to be able to remove or replace the contractor. This would be a guard against malicious contractors.

General observation

This one was quite frustrating to review. Mainly because the code is very well structured and the mechanics seem to be well thought through :) There's a lot of moving parts and metadata transactions which expand to a number of inputs ... There's also a certain level of trust required for the system to work (builder, contractor, ...) so it was hard to determine what is considered valid and what not. Was frustrating because on day 5 I thought I wouldn't find anything, so please take this as a compliment for a well structured code and a comprehensive test suite. Well done.

I'll note a couple of minor things in this QA report for your consideration.

Add event data to DisputeAttachmentAdded

Consider adding project, maybe also taskID to DisputeAttachmentAdded event as stakeholders might want to listen for this data without additional lookups. Disputes.sol L137

Add event data to LendToProject

Consider adding sender address, this is either builder or community. Project.sol L212

Rename _task

_task could be named _taskID for consistency and clarity. Project.sol L505