Rigor Protocol contest - arcoun's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L887 https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L513-L532

Vulnerability details

Impact

The address returned by SignatureDecoder.recoverKey() is never compared with the zero address (address(0)) before comparing it with the expected address. If the expected address is also the zero address (address(0)), the signature verification will pass although the signature is invalid.

There is at least two cases where this can be used to achieve an unauthorized operation:

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Community.sol#L169-L203

In Community.addMember(), this can be used to add a member to an uninitialized community as there is no verification that the community is not created and _community.owner will be equal to address(0). When the target community will later be created, the address will still be in the _community.isMember dictionary and be able to publish a project.

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/Project.sol#L493-L536

In Project.raiseDispute(), this can be used to raise a dispute while there is no contractor assigned (contractor == address(0)) on a project.

Proof of Concept

Regarding Community.addMember():

• Call addMember with:
  • data.communityID: ID to a community which is not yet created (communityID > communityCount)
  • data.newMemberAddr: address to add to the community
  • signature: 2 signatures, the first one can be junk data, the second one is a signature of the message by data.newMemberAddr
• The first checkSignatureValidity() at line 187 will pass because _community.owner == address(0) so any invalid signature will be accepted
• The second checkSignatureValidity() at line 188 will pass because the second signature is valid
• _community.isMember[_newMemberAddr] = true will add the address to the isMember dictionary
• When the community will later be created, _community.isMember[_newMemberAddr] will not be removed and the address will have the ability to bypass the require(_community.isMember[_builder], "Community::!Member") at line 248 and publish a new project.

Regarding Project.raiseDispute():

• Call raiseDispute on a project where contractor is currently unasigned
• Set data.project to the project address and data.task = 0, signature can be junk data
• After recoverKey(), signer will be equal to address(0) as the signature is invalid
• The signature verification at lines 515-518 will pass because contractor is also equal to address(0)
• IDisputes(disputes).raiseDispute() will be called to add the dispute. The signature will also be checked there to fill _dispute.raisedBy but still not checked against address(0)

Recommended Mitigation Steps

The recommended fix is to check the return address of SignatureDecoder.recoverKey():

• A SignatureDecoder.recoverKeyorThrow() function can be added and used most of the time. This function raises an error if the return address is address(0).
• In Community.checkSignatureValidity() and Project.checkSignatureValidity(), raise an error if _address is address(0).

Low Risk Issues

1. Out-of-bounds access not verified during signatureSplit

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/libraries/SignatureDecoder.sol#L58

The NatSpec documentation of signatureSplit indicates that the pos parameter should be checked to avoid out of bounds access but this is not done by any direct or indirect caller.

https://github.com/code-423n4/2022-08-rigor/blob/main/contracts/libraries/SignatureDecoder.sol#L25

Recommended mitigation is to update the test at line 25 and add || messageSignatures.length / 65 <= pos.