Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

The `TimelockController::executeEmergencyAction()` function does not update the `_operations` mapping, which can lead to an operation being executed twice.

joicygiore

Summary

The TimelockController::executeEmergencyAction() function does not update the _operations mapping, which can lead to an operation being executed twice.

Vulnerability Details

When an operation is marked as an emergency action by an account with the EMERGENCY_ROLE, it can still be executed again after the grace period. The function executeEmergencyAction() only removes the operation ID from the _emergencyActions mapping but does not update _operations. If the operation was initially scheduled as a normal execution, it remains in _operations and can be executed again once the grace period expires.

// TimelockController::executeEmergencyAction()
function executeEmergencyAction(
address[] calldata targets,
uint256[] calldata values,
bytes[] calldata calldatas,
bytes32 predecessor,
bytes32 salt
) external payable onlyRole(EMERGENCY_ROLE) nonReentrant {
bytes32 id = hashOperationBatch(targets, values, calldatas, predecessor, salt);
if (!_emergencyActions[id]) revert EmergencyActionNotScheduled(id);
@> delete _emergencyActions[id];
for (uint256 i = 0; i < targets.length; i++) {
(bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]);
if (!success) {
if (returndata.length > 0) {
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
}
revert CallReverted(id, i);
}
}
​
emit EmergencyActionExecuted(id);
}

Since the function does not mark the operation as executed in _operations, it remains eligible for execution through executeBatch() once the grace period expires.

Poc

Add the following test to test/unit/core/governance/proposals/TimelockController.test.js and execute it:

describe("Emergency Actions Executed Twice", () => {
it("should allow an operation to be executed twice", async () => {
​
const targets = [await testTarget.getAddress()];
const values = [0];
const calldatas = [testTarget.interface.encodeFunctionData("setValue", [911])];
​
// Schedules a batch of operations
await timelock.connect(proposer).scheduleBatch(
targets,
values,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash,
MIN_DELAY
);
const operationId = await timelock.hashOperationBatch(
targets,
values,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash
);
// owner add emergency actions and execute emergency actions
await timelock.connect(owner).scheduleEmergencyAction(operationId);
await expect(
timelock.connect(owner).executeEmergencyAction(
targets,
values,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash
)
).to.emit(timelock, "EmergencyActionExecuted");
​
expect(await testTarget.value()).to.equal(911);
// When the grace period is reached, the operation will be executed again
await time.increase(MIN_DELAY);
expect( await timelock.connect(executor).isOperationReady(operationId)).to.equal(true);
​
await timelock.connect(executor).executeBatch(
targets,
values,
calldatas,
ethers.ZeroHash,
ethers.ZeroHash,
);
});
});

Tools Used

Manual Review

Recommendations

To prevent duplicate execution, update the _operations mapping when executing emergency actions. Specifically, set the executed flag to true when an operation is executed via executeEmergencyAction().

function executeEmergencyAction(
address[] calldata targets,
uint256[] calldata values,
bytes[] calldata calldatas,
bytes32 predecessor,
bytes32 salt
) external payable onlyRole(EMERGENCY_ROLE) nonReentrant {
bytes32 id = hashOperationBatch(targets, values, calldatas, predecessor, salt);
if (!_emergencyActions[id]) revert EmergencyActionNotScheduled(id);
delete _emergencyActions[id];
+ _operations[id].executed = true; // Mark the operation as executed
for (uint256 i = 0; i < targets.length; i++) {
(bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]);
if (!success) {
if (returndata.length > 0) {
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
}
revert CallReverted(id, i);
}
}
​
emit EmergencyActionExecuted(id);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

TimelockController.executeEmergencyAction doesn't mark operations as executed, allowing the same operation to be executed again through the regular path

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

TimelockController.executeEmergencyAction doesn't mark operations as executed, allowing the same operation to be executed again through the regular path

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.