MultiSig Timelock
First Flight #55
Beginner FriendlyWallet
100 EXP
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: low

Missing assertions in testSignerCanRevokeConfirmation() leads to false positives

Author Revealed upon completion

Description

The test suite contains a critical "smoke test" titled testSignerCanRevokeConfirmation. While the test executes the full flow of proposing, confirming, and revoking a transaction, it lacks an Assert phase.

In its current state, the test only verifies that the function calls do not revert. It fails to verify that the internal state of the contract—specifically the confirmations count is accurately updated after a revocation occurs.

Risk

Low severity. The lack of assertions in the test suite creates a False Sense of Security, where the test suite provides "green" results despite potential logic failures. This constitutes a Testing Gap that allows for Undetected Logic Regressions.

If a developer accidentally breaks the revokeConfirmation logic—for example, by failing to decrement the confirmation count or failing to reset the timelock timestamp, the test will still report a "Pass" because it only checks for the absence of a revert. This bypasses the primary purpose of a CI/CD pipeline, which is to ensure that state transitions remain correct after code changes..

Impact

Unauthorized Transaction Execution via "Stale" Confirmations. The failure to assert that the confirmations count decrements during a revoke leads to a critical vulnerability where the Security Threshold of the MultiSig is compromised.
Bypassing Quorum Requirements: If the counter does not decrement when a signer revokes their vote, the transaction retains "stale" confirmations. This allows a transaction to reach the required quorum (e.g., 3/5) with fewer than the required number of active supporters.
Unauthorized Fund Transfer: A transaction that was intended to be cancelled or paused by revoking votes could still be executed. This effectively allows a minority of signers to push through a transaction that the majority no longer supports, leading to unauthorized movement of protocol funds.
Irreversible State Changes: Because the test suite doesn't verify the decrement, a bug in the revocation logic would go unnoticed until a real-world scenario where a revoked vote still counts toward execution, at which point the damage (lost funds or changed contract ownership) is already done.

PoC (Proof of Concept)

If the body of the revokeConfirmation function in the contract were completely deleted (empty brackets { }), the following test would still pass:

Solidity

function testSignerCanRevokeConfirmation() public grantSigningRoles {
// ... ARRANGE & ACT logic ...
// The test ends here without checking if the logic actually worked.
// Even if confirmations remained at 5 instead of 0, the test passes.
}

Recommended Mitigation

Update the test to include an assertion that verifies the transaction state. Specifically, ensure that the confirmations count returns to the expected value after the revocation calls.

Solidity

function testSignerCanRevokeConfirmation() public grantSigningRoles {
// ... [EXISTING ARRANGE/ACT] ...
​
// ASSERT
// This assertion makes sure the comfirmations reset, matching contract logic
assertEq(multiSigTimelock.getTransaction(txnId).confirmations, 0);
}

Support

FAQs

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

Give us feedback!