hats-finance / Most--Aleph-Zero-Bridge-0xab7c1d45ae21e7133574746b2985c58e0ae2e61d

Aleph Zero bridge to Ethereum
Apache License 2.0
0 stars 1 forks source link

Inadequate Address Check in `eth::most::_setCommittee` #35

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xbd3d01e65005eaa3684f2165538d92f26e27c6919fdec34b8d65cb6bcd3eaafe Severity: minor

Description: Description
The _setCommittee function lacks a check against address 0 for each address in _committee. If any of these addresses are set to 0, it may lead to a temporary lock of funds for the specific committeeId as the threshold may not be reached.

Impact
Temporary freezing of funds for that specific commit ID. However, this issue is recoverable by the owner.

Scenario
Suppose the owner wants to set a new committee with a threshold of 2 committee members. If one of them is set to address zero, all receive requests of that committee will not pass the threshold.

POC

pragma solidity ^0.8.20;

import "forge-std/Test.sol"; import "forge-std/console.sol"; import "../contracts/Most.sol";

contract POC is Test { Most public most;

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

address alice = makeAddr("alice");

address public WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;
address public WBTC = 0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599;

function test_poc() public {

    //setup threshold is 2 but one of addresses is address(0)
    address[] memory addresses = new address[](2);
    addresses[0] = alice;
    addresses[1] = address(0);
    most.initialize(addresses, 2, alice, payable(WETH));

    vm.startPrank(alice);
    most.addPair(bytes32(uint256(uint160(WETH))), bytes32(uint256(uint160(WBTC))));
    most.unpause();
    vm.deal(alice, 1 ether);
    most.sendRequestNative{value: 1 ether}(bytes32(uint256(uint160(alice))));
    uint256 _committeeId = 0;
    bytes32 destTokenAddress = bytes32(uint256(uint160(WETH)));
    uint256 amount = 1 ether;
    bytes32 destReceiverAddress = bytes32(uint256(uint160(alice)));
    uint256 _requestNonce = 0;
    bytes32 requestHash =
        keccak256(abi.encodePacked(_committeeId, destTokenAddress, amount, destReceiverAddress, _requestNonce));

    most.receiveRequest(requestHash, _committeeId, destTokenAddress, amount, destReceiverAddress, _requestNonce);
    /// now it has 1 approval, but threshold is 2
}

}


**Revised Code File (Optional)**  
Implement a check for each address in `_committee` against address 0.

```diff
         for (uint256 i; i < _committee.length; ++i) {
             bytes32 committeeMemberId = keccak256(abi.encodePacked(committeeId, _committee[i]));
+            if (_committee[i] == address(0)) {
+                revert();
+            }
krzysztofziobro commented 4 months ago

This assumes the owner error - out of scope