Impact: H
Likelihood: H
Root + Impact
Description
The protocol introduced delays for ETH transfers to prevent rushed or potentially compromised high-value transfers. However, the execution time is tied to the transaction proposal timestamp, rather than to the time when quorum was reached.
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:
Impact:
-
The 'cool-down' period is compromised, so the harmful proposal may be executed without review.
-
The signer will not be able to revoke confirmation if he signed a transaction too close to proposal's due time - the purpose of delay is neglected here.
Proof of Concept
Please, add the following test to MultiSigTimeLockTest.sol. Kindly check a transaction lifecycle scheme to explain that executeTransaction function was successful 1 minute later the last confirmation.
As can be seen, the 1 minute period of time is not enough to review/revoke possibly harmful transaction.
function test_fuzz_signerHas1minute_toChangeHisMind_beforeExecuteTransaction(uint256 ethAmount) public grantSigningRoles {
TRANSACTION LIFECYCLE
delay = (timeDelayForAmount - 1 minute) delay = 1 minute
--------|--------------------------------------------------------------------------|------------------------------|-----
proposeTransaction 3rd confirmation executeTransaction
and 2 confirmations comes 1 minute before
done sequentially execution
*/
ethAmount = bound(ethAmount, 1 ether, 200 ether);
testTimelockDelay = new TestTimelockDelay();
uint256 timeDelayForAmount = testTimelockDelay.getTimelockDelay(ethAmount);
vm.prank(OWNER);
uint256 txnId = multiSigTimelock.proposeTransaction(SPENDER_ONE, ethAmount, hex"");
vm.prank(OWNER);
multiSigTimelock.confirmTransaction(txnId);
vm.prank(SIGNER_TWO);
multiSigTimelock.confirmTransaction(txnId);
uint256 startOfDelay = block.timestamp;
vm.warp(startOfDelay + timeDelayForAmount - 1 minutes);
vm.prank(SIGNER_FIVE);
multiSigTimelock.confirmTransaction(txnId);
uint256 timeWhenConfirmed = block.timestamp;
vm.warp(timeWhenConfirmed + 1 minutes);
uint256 endOfDelay = block.timestamp;
uint256 elapsed = endOfDelay - timeWhenConfirmed;
assertLe(elapsed, 1 minutes);
MultiSigTimelock.Transaction memory txn = multiSigTimelock.getTransaction(txnId);
vm.prank(OWNER);
vm.deal(address(multiSigTimelock), ethAmount);
vm.expectEmit();
emit MultiSigTimelock.TransactionExecuted(txnId, txn.to, txn.value);
multiSigTimelock.executeTransaction(txnId);
}
Recommended Mitigation
Timelock should start when the quorum is reached:
-
Add a member lastConfirmedTimestamp to MultiSigTimelock::Transaction the struct.
-
Update lastConfirmedTimestamp in _confirmTransaction function.
-
Check this timestamp when trying to execute the transaction instead of proposedAt timestamp.
struct Transaction {
address to;
uint256 value;
bytes data;
uint256 confirmations;
+ uint256 lastConfirmedTimestamp;
uint256 proposedAt;
bool executed;
}
function _confirmTransaction(uint256 txnId) internal {
if (s_signatures[txnId][msg.sender]) {
revert MultiSigTimeLock__UserAlreadySigned();
}
s_signatures[txnId][msg.sender] = true;
// Increase counter
s_transactions[txnId].confirmations++;
+ // update lastConfirmedTimestamp
+ s_transactions[txnId].lastConfirmedTimestamp = block.timestamp;
emit TransactionConfirmed(txnId, msg.sender);
}
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;
+ uint256 executionTime = txn.lastConfirmedTimestamp + requiredDelay;
if (block.timestamp < executionTime) {
revert MultiSigTimelock__TimelockHasNotExpired(executionTime);
}
// other code is removed for clarity
}
