Can add member and publish project to future community without community owner approval

[code423n4]

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, ...).

[parv3213]

Duplicate of #298