032.md

Energetic Plum Lion

High

"Approval Mechanism in MultiSign.sol: Handling Pauses, Unpauses, and Overlapping Approvals"

Summary

In the MultiSign.sol contract, owners can approve both pause and unpause for the same function in different rounds. However, when executing executePause or executeUnPause, only the respective mapping (either pauseApproved or unpauseApproved) is cleared, and the approvals in the other mapping remain intact.

Root Cause

In the MultiSign.sol contract, when approving the contract functions for pause and unpause, an owner can approve the same function twice, i.e., they can approve pause while also approving unpause.

However, when executing executePause or executeUnPause, only the corresponding mappings' states will be cleared.

    function executePause(Functions _function) private returns (bool) {
        // Get the number of approvals
        require(getApprovalPauseCount(_function) >= requiredApprovals,"Required approvals not met");
        // Loop through the approved functions with owners mapping and change to false
        for (uint64 i; i < noOfOwners; i++) {
            pauseApproved[_function][owners[i]] = false;
        }
        return true;
       }
	function executeUnPause(Functions _function) private returns (bool) {
        // Get the number of approvals
        require(getApprovalUnPauseCount(_function) >= requiredApprovals,"Required approvals not met");
        // Loop through the approved functions with owners mapping and change to false
        for (uint64 i; i < noOfOwners; i++) {
            unpauseApproved[_function][owners[i]] = false;
        }
        return true;
    }

When executing executePause, the pauseApproved mapping for owners will be cleared, but the unpauseApproved mapping for the same owners will not be cleared and will remain. Additionally, owners cannot revoke their previous approvals until the executePause or executeUnPause steps are executed, which clears the previous round of approvals. Therefore, if ownerA approves Pause in the first round, but unPause is executed in the end, ownerA's approval for Pause will remain until executePause is executed. This means that in the next round of approvals, any unexecuted approvals from the previous round will still persist. Thus, ownerA may approve both unPause and Pause in the same round.

code snippet 1 ,code snippet 2

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

The impact of the approval mechanism in the MultiSign.sol contract is that owners' previous approvals for pause and unpause functions can persist across rounds, leading to unintended overlaps. This could cause confusion or security risks, as owners may accidentally approve both actions for the same function, or their approvals might remain valid even if the corresponding action was not executed. Additionally, owners cannot revoke their approvals until the respective executePause or executeUnPause functions are executed, making it harder to manage or correct approvals in a timely manner.

PoC

//owner A ,B,C,D,E   requiredApprovals==3;
//Round 1  A(approveUnPause),B(approveUnPause),C(approvePause),D(approvePause),E(approvePause)
--------->Execute executePause:
The approval statuses of the owners become---------->A(approveUnPause),B(approveUnPause),C(),D(),E()
//Round 2  A(approvePause),B(approvePause),C(approveUnPause),D(approvePause),E(approvePause)
--------->Only one approval from C, D, or E is required to execute executeUnPause.

Mitigation

Clear all approvals in the executePause or executeUnPause functions.

The original code is as follows:

function executePause(Functions _function) private returns (bool) {
    // Get the number of approvals
    require(getApprovalPauseCount(_function) >= requiredApprovals,"Required approvals not met");
    // Loop through the approved functions with owners mapping and change to false
    for (uint64 i; i < noOfOwners; i++) {
        pauseApproved[_function][owners[i]] = false;
    }
    return true;
   }
function executeUnPause(Functions _function) private returns (bool) {
    // Get the number of approvals
    require(getApprovalUnPauseCount(_function) >= requiredApprovals,"Required approvals not met");
    // Loop through the approved functions with owners mapping and change to false
    for (uint64 i; i < noOfOwners; i++) {
        unpauseApproved[_function][owners[i]] = false;
    }
    return true;
}

Change to:

function executePause(Functions _function) private returns (bool) {
    // Get the number of approvals
    require(getApprovalPauseCount(_function) >= requiredApprovals,"Required approvals not met");
    // Loop through the approved functions with owners mapping and change to false
    for (uint64 i; i < noOfOwners; i++) {
        pauseApproved[_function][owners[i]] = false;
        unpauseApproved[_function][owners[i]] = false;
    }
    return true;
   }
function executeUnPause(Functions _function) private returns (bool) {
    // Get the number of approvals
    require(getApprovalUnPauseCount(_function) >= requiredApprovals,"Required approvals not met");
    // Loop through the approved functions with owners mapping and change to false
    for (uint64 i; i < noOfOwners; i++) {
        pauseApproved[_function][owners[i]] = false;
        unpauseApproved[_function][owners[i]] = false;
    }
    return true;
}