Locked tokens could be locked forever

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xa36057f836b08dd81c132c5b6444a68d2153ee7a93684bd633b041558dd13f37 Severity: critical

Description: Description\ The _setCommittee function only reverts when _committee.length is greater than _signatureThreshold.

It doesn't revert when _signatureThreshold is greater than _committee.length. Test on this is in the PoC section.

This is a critical issue in the receiveRequest function when _signatureThreshold is higher than _committee.length.

In receiveRequest function, for the locked tokens to be returned, signature count of committee members must be greater than or equal to signatureThreshold of a committeeId.

if (request.signatureCount >= signatureThreshold[_committeeId])

Say if a committeeId has 5 members but 6 signatureThreshold, it would be impossible to return locked tokens by calling the receiveRequest function.

1. Proof of Concept (PoC) File

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";

contract Committee {
    uint256 public committeeId;
    mapping(bytes32 => bool) private committee;
    mapping(uint256 => uint256) public committeeSize;
    mapping(uint256 => uint256) public signatureThreshold;

    event CommitteeUpdated(uint256 _committeeId);

    error ZeroSignatureTreshold();
    error NotEnoughGuardians();
    error DuplicateCommitteeMember();

    function setCommittee(
        address[] calldata _committee,
        uint256 _signatureThreshold
    ) external /*onlyOwner*/ {
        ++committeeId;
        _setCommittee(_committee, _signatureThreshold);
        emit CommitteeUpdated(committeeId);
    }

    function _setCommittee(
        address[] calldata _committee,
        uint256 _signatureThreshold
    ) internal {
        if (_signatureThreshold == 0) revert ZeroSignatureTreshold();
        if (_committee.length < _signatureThreshold)
            revert NotEnoughGuardians();

        for (uint256 i; i < _committee.length; ++i) {
            bytes32 committeeMemberId = keccak256(
                abi.encodePacked(committeeId, _committee[i])
            );

            // avoid duplicates
            if (committee[committeeMemberId]) {
                revert DuplicateCommitteeMember();
            }

            committee[committeeMemberId] = true;
        }

        committeeSize[committeeId] = _committee.length;
        signatureThreshold[committeeId] = _signatureThreshold;
    }
}

contract CommitteeTest is Test {
    address user = vm.addr(1);
    Committee public committee;

    function setUp() public {
        committee = new Committee();
    }

    function testSignatureThresholdTooHigh() public {
        address[] memory members = new address[](3);
        members[0] = address(0x1);
        members[1] = address(0x2);
        members[2] = address (0x3);

        vm.prank(user); // Impersonate the contract owner
       // vm.expectRevert();
        committee.setCommittee(members, 2);
    }
}

Here's the output:

forge test -vvvv [⠔] Compiling... [⠘] Compiling 1 files with 0.8.22 [⠃] Solc 0.8.22 finished in 1.25s Compiler run successful!

Running 1 test for test/A.t.sol:CommitteeTest [PASS] testSignatureThresholdTooHigh() (gas: 148231) Traces: [148231] CommitteeTest::testSignatureThresholdTooHigh() ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf) │ └─ ← () ├─ [137180] Committee::setCommittee([0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000003], 2) │ ├─ emit CommitteeUpdated(_committeeId: 1) │ └─ ← () └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 813.03µs

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

2. Revised Code File (Optional)

[ololade97]

**Please disregard this submission

[krzysztofziobro]

This is both recoverable by owner + requires owner error