Root + Impact
Description
The MultiSigTimelock shares an ETH pool among multiple transactions without requiring reservation of funds during transaction proposal and without tracking these reserves at execution time. This creates a situation where one transaction might drain all the funds, leaving other transactions without any ETH to transfer and causing a denial-of-service due to insolvency.
The _proposeTransaction function does not reserve funds upon proposal, and _executeTransaction does not reduce the reserved amount when a transaction is executed.
function _proposeTransaction(address to, uint256 value, bytes memory data) internal returns (uint256) {
@>
uint256 transactionId = s_transactionCount;
s_transactions[transactionId] = Transaction({
to: to, value: value, data: data, confirmations: 0, proposedAt: block.timestamp, executed: false
});
s_transactionCount++;
emit TransactionProposed(transactionId, to, value);
return transactionId;
}
.
.
.
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
if (txn.value > address(this).balance) {
revert MultiSigTimelock__InsufficientBalance(address(this).balance);
}
txn.executed = true;
@>
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) {
revert MultiSigTimelock__ExecutionFailed();
}
emit TransactionExecuted(txnId, txn.to, txn.value);
}
Risk
Likelihood:
-
Since different transaction values result in different delays, this situation is highly likely, as some funds mightremain reserved until execution time.
-
Some signers might not want to reserve funds for a proposed transaction and instead wait to execute it when thecontract has sufficient funds, thereby using funds that might have been reserved for a specific other transaction.
Impact:
Proof of Concept
Add this snippet of code to the MultiSigTimelockTest.t.sol test file."
function test_TransactionsDrainSharedPoolCauseDenialOfService() public grantSigningRoles {
vm.deal(address(multiSigTimelock), OWNER_BALANCE_TWO);
console2.log("OWNER BALANCE: ", OWNER.balance);
console2.log("SPENDER_ONE BALANCE: ", SPENDER_ONE.balance);
console2.log("SPENDER_TWO BALANCE: ", SPENDER_TWO.balance);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, OWNER_BALANCE_TWO, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId);
vm.warp(block.timestamp + 1 days + 1);
vm.prank(OWNER);
uint256 txnId2 = multiSigTimelock.proposeTransaction(SPENDER_TWO, OWNER_BALANCE_ONE, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId2);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId2);
vm.prank(SIGNER_THREE);
multiSigTimelock.confirmTransaction(txnId2);
vm.prank(OWNER);
multiSigTimelock.executeTransaction(txnId);
vm.prank(OWNER);
vm.expectRevert(
abi.encodeWithSelector(
MultiSigTimelock.MultiSigTimelock__InsufficientBalance.selector, address(multiSigTimelock).balance
)
);
multiSigTimelock.executeTransaction(txnId2);
console2.log("\nAfter executing first transaction of 5 ether to SPENDER_ONE");
console2.log("SPENDER_ONE BALANCE: ", SPENDER_ONE.balance);
console2.log("SPENDER_TWO BALANCE: ", SPENDER_TWO.balance);
}
How to execute:
forge test --mt test_TransactionsDrainSharedPoolCauseDenialOfService -vv
Recommended Mitigation
Add funds reservation to the _proposeTransaction function, and reduce the reserved funds value for executed transactions in the _executeTransaction function.
+ uint256 private s_ethReservation; // track ETH reserved for proposed transactions
.
.
.
function _proposeTransaction(address to, uint256 value, bytes memory data) internal returns (uint256) {
+ uint256 transactionId = s_transactionCount;
+ s_ethReservation += value;
+ if (s_ethReservation > address(this).balance) {
+ revert MultiSigTimelock__InsufficientBalance(address(this).balance);
+ }
- uint256 transactionId = s_transactionCount;
s_transactions[transactionId] = Transaction({
to: to, value: value, data: data, confirmations: 0, proposedAt: block.timestamp, executed: false
});
s_transactionCount++;
emit TransactionProposed(transactionId, to, value);
return transactionId;
}
.
.
.
function _executeTransaction(uint256 txnId) internal {
Transaction storage txn = s_transactions[txnId];
// CHECKS
// 1. Check if enough confirmations
if (txn.confirmations < REQUIRED_CONFIRMATIONS) {
revert MultiSigTimelock__InsufficientConfirmations(REQUIRED_CONFIRMATIONS, txn.confirmations);
}
// 2. Check if timelock period has passed
uint256 requiredDelay = _getTimelockDelay(txn.value);
uint256 executionTime = txn.proposedAt + requiredDelay;
// q unsafe comparison ?
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
if (txn.value > address(this).balance) {
revert MultiSigTimelock__InsufficientBalance(address(this).balance);
}
// EFFECTS
// 3. Mark as executed BEFORE the external call (prevent reentrancy)
txn.executed = true;
- // INTERACTIONS
+ s_ethReservation -= txn.value;
+ // INTERACTIONS
// 4. Execute the transaction
(bool success,) = payable(txn.to).call{value: txn.value}(txn.data);
if (!success) {
revert MultiSigTimelock__ExecutionFailed();
}
// 5. Emit eventt
emit TransactionExecuted(txnId, txn.to, txn.value);
}
