Previous committee members can still call receiveRequest after committee update

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xc95183fd944036b5ebd9bf5050e65e562ca16f3598bdcf346f8fac81d05b37dd Severity: high

Description: Description\ In Most.sol, there is a bug where the previous committeeId and committee members can still call receiveRequest after updating the committee to a new set via setCommittee.

The issue is that setCommittee is not properly clearing the previous committee mapping. So the previous members are still marked as being in the committee.

This allows previous committee members to still call receiveRequest() even after the committee has changed. This could lead to unintended approvals by members who should no longer have permission.

1. Proof of Concept (PoC) File

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

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

contract Most {

    uint256 public committeeId;

    mapping(bytes32 committeeMemberId => bool) private committee;
    mapping(uint256 committeeId => uint256) public committeeSize;
    mapping(uint256 committeeId => uint256) public signatureThreshold;

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

    event CommitteeUpdated(uint256 newCommitteeId);

    modifier _onlyCommitteeMember(uint256 _committeeId) {
        if (!isInCommittee(_committeeId, msg.sender)) revert NotInCommittee();
        _;
    }

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

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

    function receiveRequest(
         uint256 _committeeId,
        uint256 amount
    ) external /*whenNotPaused*/ _onlyCommitteeMember(_committeeId) {
        //do something
    }

      function isInCommittee(
        uint256 _committeeId,
        address account
    ) public view returns (bool) {
        return committee[keccak256(abi.encodePacked(_committeeId, account))];
    }

}

contract MostTest is Test {
    Most public most;

    address pastMember = vm.addr(1);
    address presentMember = vm.addr(2);
    address owner = vm.addr(3);

    function setUp() public {
        most = new Most();
    }

    function testReceiveRequest() public {
        vm.prank(owner);
        address[] memory member = new address[](1);
        member[0] = pastMember;        
        most.setCommittee(member, 1);
        vm.stopPrank();

        vm.prank(owner);
        address[] memory member2 = new address[](1);
        member2[0] = presentMember;
        most.setCommittee(member2, 1);
        vm.stopPrank();

        vm.prank(pastMember);
        most.receiveRequest(1, 2);
        vm.stopPrank();
    }

    function testIsinCommttee() public {
        vm.prank(pastMember);
        address[] memory member = new address[](1);
        member[0] = pastMember;        
        most.setCommittee(member, 1);
        vm.stopPrank();

        vm.prank(presentMember);
        address[] memory member2 = new address[](1);
        member2[0] = presentMember;
        most.setCommittee(member2, 1);
        vm.stopPrank();
        most.isInCommittee(1, pastMember);
    }
}

Here's the result:

forge test -vvvv [⠒] Compiling... [⠰] Compiling 1 files with 0.8.22 [⠔] Solc 0.8.22 finished in 1.69s Compiler run successful!

Running 2 tests for test/A.t.sol:MostTest [PASS] testIsinCommttee() (gas: 177434) Traces: [177434] MostTest::testIsinCommttee() ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf) │ └─ ← () ├─ [91382] Most::setCommittee([0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf], 1) │ ├─ emit CommitteeUpdated(newCommitteeId: 1) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () ├─ [0] VM::prank(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) │ └─ ← () ├─ [69482] Most::setCommittee([0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF], 1) │ ├─ emit CommitteeUpdated(newCommitteeId: 2) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () ├─ [777] Most::isInCommittee(1, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf) [staticcall] │ └─ ← true └─ ← ()

[PASS] testReceiveRequest() (gas: 180105) Traces: [180105] MostTest::testReceiveRequest() ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) │ └─ ← () ├─ [91382] Most::setCommittee([0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf], 1) │ ├─ emit CommitteeUpdated(newCommitteeId: 1) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) │ └─ ← () ├─ [69482] Most::setCommittee([0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF], 1) │ ├─ emit CommitteeUpdated(newCommitteeId: 2) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf) │ └─ ← () ├─ [762] Most::receiveRequest(1, 2) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () └─ ← ()

Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 78.81ms

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

[ololade97]

I believe this is bug is "critical" and not a "high". This is because it can result into what is classified as critical on the contest page:

Unauthorized unlocking (on Ethereum) of tokens or minting (on Aleph Zero) of bridged tokens, resulting in a significant drop of bridge TVL (>5k USD value) and thus noticeable protocol loss.

[krzysztofziobro]

You perform two committee updates:

• committee_id = 1, one member pastMember
• committee_id = 2, one member currentMember

Then in your test you check whether pastMember is a member of the committee 1, which is the past committee, and you get that he indeed is, which is a correct behavior.

I don't see any issue here.

[ololade97]

The issue is that a past committee member shouldn't be able to call receiveRequest function when there is an update to committee members.

When there is an update to committee members, past members shouldn't be committee members anymore.

If past members are still part of the committee, updating committee through the setCommittee function has no essence.

[ololade97]

In the bug pointed out, even when a committee is changed, past members can still call the receiveRequest function. And this is critical.

[krzysztofziobro]

They are part of the previous committee, so they can call receiveRequest on past requests, but not on the new ones, which is the intended behavior (note that the corresponding CommitteeId is a part of the request).

[ololade97]

Past members can also can the function on present request.

It doesn't matter which committeeId is calling the function. Funds can be removed at any time whether by past or present member or committeeId.

[krzysztofziobro]

You mean that 'threshold' of the members of the past committee can move the funds? That's true, but that requires 'threshold' of cooperating malicious members in the committee at some point (for the funds to be stolen).

[ololade97]

You mean that 'threshold' of the members of the past committee can move the funds? That's true, but that requires 'threshold' of cooperating malicious members in the committee at some point (for the funds to be stolen).

Yes, you're completely right.

[ololade97]

I could see the report is closed?

[krzysztofziobro]

Yes, as any attacks that require at least threshold malicious committee members are out of scope

[ololade97]

But these are past committees (invariably no longer members, not the present?

[krzysztofziobro]

Past committee is also a committee, such a committee isn't supposed to operate outside confirming the old requests that have not been yet processed for some reason, but in theory there is no reason why such a committee would not be able to move the funds, and that's the intended behavior of the contracts

[ololade97]

My contention is past committee members can move funds belonging to any committee or committee.

All that would need to change in receiveRwquest is the _requestHash, amount, and nonce.

[krzysztofziobro]

That's not demonstrated in the PoC and I don't believe it to be true, as _requestHash is validated against the request data, which includes the CommitteeId

[ololade97]

The PoC demonstrated that past members can still call receiveRequest function after there's an update in committee.

The first issue is, why should there be an update when past members haven't concluded their duty?

The second issue is, past members can pass andy value as parameters. The requestHash you mentioned can be bypassed as a result.

With the bug pointed out, it doesn't matter the committeeId a member belongs to, funds can be withdrawn from the contract because there is freedom to change the parameter inputs.

[ololade97]

Also note that the setCommittee function has the "whenPaused" modifier.

So the question is, should past members be able to call the receiveRequest function after an update?