Uninitialized state allows any Party member to validate signatures using the OffChainSignatureValidator

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/0ce3819de173f7688c9c834ce2cc758dd03c9bd2/contracts/signature-validators/OffChainSignatureValidator.sol#L72

Vulnerability details

Summary

After a Party has been created, the default signing threshold in OffChainSignatureValidator allows any member with a minimal fraction of voting power to validate signatures.

Impact

The implementation of isValidSignature() in the OffChainSignatureValidator contract uses a Party configured threshold to control signing. If the signer's voting power is above this threshold, the signature is validated:

https://github.com/code-423n4/2023-10-party/blob/0ce3819de173f7688c9c834ce2cc758dd03c9bd2/contracts/signature-validators/OffChainSignatureValidator.sol#L57-L79

57:         Party party = Party(payable(msg.sender));
58:         address signer = ecrecover(hash, v, r, s);
59:         uint96 signerVotingPowerBps = party.getVotingPowerAt(signer, uint40(block.timestamp)) *
60:             10000;
61: 
62:         if (signerVotingPowerBps == 0 && party.balanceOf(signer) == 0) {
63:             // Must own a party card or be delegatated voting power
64:             revert NotMemberOfParty();
65:         }
66: 
67:         uint96 totalVotingPower = party.getGovernanceValues().totalVotingPower;
68:         uint96 thresholdBps = signingThersholdBps[party];
69: 
70:         // Either threshold is 0 or signer votes above threshold
71:         if (
72:             thresholdBps == 0 ||
73:             (signerVotingPowerBps > totalVotingPower &&
74:                 signerVotingPowerBps / totalVotingPower >= thresholdBps)
75:         ) {
76:             return IERC1271.isValidSignature.selector;
77:         }
78: 
79:         revert InsufficientVotingPower();

As we can see in the previous snippet of code, if the threshold is zero, then the signature will be validated as long as the signer is a party member, independently of the voting power they hold.

This threshold can be configured by Parties by calling the setSigningThresholdBps() function. As stated in the docs, this function needs to be actively called from the Party by creating a proposal (using an ArbitraryCallsProposal):

This function sets the signing threshold for the caller —- as such, it must be called by the Party in an ArbitraryCallsProposal.

The default threshold value is zero, which is the default value for the signingThersholdBps mapping. This means that after a Party has been created, and up to the formal proposal is created and executed, the signing threshold will be zero and would allow any Party member to validate signatures using the OffChainSignatureValidator contract.

Proof of Concept

The following test reproduces the issue. Here, Steve, who holds just 1 voting power, is able to sign any message and have it validated using the OffChainSignatureValidator.

Note: this test should be run in the context of the OffChainSignatureValidator.t.sol file.

function test_Party_UninitializedSignThreshold() public {
    // Steve has just 1 voting power
    (bytes32 messageHash, bytes memory signature) = _signMessage(
        stevePk,
        "Hello World! nonce:1000"
    );

    bytes memory staticCallData = abi.encodeWithSelector(
        IERC1271.isValidSignature.selector,
        messageHash,
        signature
    );
    vm.startPrank(address(0), address(0));
    (bool success, bytes memory res) = address(party).staticcall(staticCallData);
    assertTrue(success);
    assertEq(abi.decode(res, (bytes4)), IERC1271.isValidSignature.selector);
}

Recommendation

During Party creation, add an option in the PartyOptions structure to define the signing threshold. When initialized, the Party could call the registered global OffChainSignatureValidator (GLOBAL_OFF_CHAIN_SIGNATURE_VALIDATOR) to set the desired initial value for the signing threshold.

Assessed type

Other

[ydspa]

QA: L

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[c4-pre-sort]

ydspa marked the issue as primary issue

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)