Delete call on ``pendingRequests[hash]`` does not clear the nested mapping for signatures

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x2979e5f798a89526d8a146d42d23b84015aaca4ea9b85f36b14dc068a5b078bb Severity: minor

Description: Description\ The receiveRequest acts as a way for the relayers/committee members to vote on the execution on cross-chain requests. When a request passess quorum and gets execute, it gets added to a mapping of processed requests and gets deleted from the mapping of pending requests. The problem lies in the fact that the pendingRequests mapping contains structs with a nested mapping in them.

Attack Scenario\ The solidity language's delete keyword is used to reset the values of variables to the default values for their types, thus: For uint/int -> 0 For bool -> false For arrays -> all elements get reset For address/bytes32 -> 0x0..... But for mappings the story is different, since the delete operation does not do anything to mappings of whatever type - they remain the same. This is due to it being impossible to find every used key for the mapping and resetting it's value to default, thus solidity ignores mappings. The same principle is true for structs - deleting a struct resets all it's members except mappings, since their occupied keys are impossible to track.

In the context of the current codebase, the line:

delete pendingRequests[requestHash];

would only reset the signatureCount member for the hash to 0, but the mapping signatures would remain uncleared, leaving the contract in an unexpected state. This means that any attempt to re-execute a request, due to some committee decision or re-attempt of a failed request, would be impossible with the previous committee members.

This leaves 2 instantly notable impacts, classifying for Minor severity:

Old request, even if needed, would become unexecutable by the same committee, since their signature fields would remain uncleared (set to true). In the scenario where commiteeCount / 2 < signaturesThreshold the request cannot be re-executed at all.
The view function hasSignedRequest() would return wrong results on already processed hashes(true instead of false), potentially tampering with the off-chain monitoring and UX.

Attachments

Proof of Concept (PoC) File


// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19.0;

import {Test, console} from "forge-std/Test.sol"; import "forge-std/console.sol"; import {Most} from "../contracts/Most.sol"; import {Token} from "../contracts/Token.sol";

contract NoReturnTest is Test { Most m; address user = address(1); address receiver = address(2); address[] commitee = new address;

Token mockToken;

event TokenTransferFailed(bytes32 requestHash);

function setUp() public {
    commitee[0] = user;
    m = new Most();
    m.initialize(commitee, 1, address(this), payable(0));
    mockToken = new Token(1_000_000, 18, "USDT-Mock", "USDTM");
    mockToken.transfer(user, 10_000);

    m.unpause();
}

function test_unclearedMappings() public {
    vm.startPrank(user);

    bytes32 requestHash = keccak256(
        abi.encodePacked(
            uint256(0),
            bytes32(uint256(uint160(address(mockToken)))),
            uint256(1000),
            bytes32(uint256(uint160(address(receiver)))),
            uint256(0)
        )
    );

    m.receiveRequest(
        requestHash, 
        0,
        bytes32(uint256(uint160(address(mockToken)))),
        1000,
        bytes32(uint256(uint160(address(receiver)))),
        0
    );

    (uint256 count) = m.pendingRequests(requestHash);

    assertEq(count, 0);
    assertEq(m.hasSignedRequest(user, requestHash), true);
}

}


The following test showcases that when a request gets received and proccessed successfully, the ``signatureCount`` of the request is reset to 0 successfully, but the call to ``hasSignedRequest`` remains true, even though it should.

2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->

Recommendaton: a way to avoid this would be to refactor how already voted members are tracked - the best is an array, since it can get easily cleared on the ``delete`` call. 
Avoid nesting mappings inside of structs if you will be attempting to delete them later on

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

Delete call on ``pendingRequests[hash]`` does not clear the nested mapping for signatures #37