DatingDapp

AI First Flight #6

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Impact: low

Likelihood: low

Invalid

MultiSigWallet approvals cannot be revoked and transactions never expire or get cancelled, so after one owner approves, the other can approve and execute that transaction at any later time on consent that may no longer be current

friedsocks233

Root + Impact

Description

The match wallet is a 2-of-2: ETH leaves only through a transaction that both owners approve (L72). Suppose an owner approves a transaction for a date that is later cancelled. They should be able to withdraw that approval before the transaction executes.
approveTransaction only ever sets approvedByOwner1 or approvedByOwner2 to true (L58, L61), and calling it again reverts AlreadyApproved. No function clears an approval or cancels a pending transaction. The Transaction struct (L14-L20) has no deadline, and executeTransaction checks only !executed and the two flags (L71-L72). Once one owner approves, the other owner alone decides when the transaction runs; in the PoC that is a year later.

The amount is fixed at submission (L45, L75). A stale transaction therefore sends exactly the approved value to the approved recipient, never "the wallet balance". A larger balance can only make an approved transaction that was unfunded at the time executable later.

// src/MultiSig.sol L14-L20: a transaction has no deadline or expiry

@> struct Transaction {

address to;

uint256 value;

bool approvedByOwner1;

bool approvedByOwner2;

bool executed;

}

// src/MultiSig.sol L56-L62: approvals can only be set to true; no function clears one or cancels a transaction

if (msg.sender == owner1) {

if (txn.approvedByOwner1) revert AlreadyApproved();

@> txn.approvedByOwner1 = true;

} else {

if (txn.approvedByOwner2) revert AlreadyApproved();

@> txn.approvedByOwner2 = true;

}

// src/MultiSig.sol L71-L72: execution checks only the flags, with no time component

@> require(!txn.executed, "Transaction already executed");

@> require(txn.approvedByOwner1 && txn.approvedByOwner2, "Not enough approvals");

Risk

Likelihood:

It happens when one owner approves a transaction and then changes their mind before the other owner co-signs, and the other owner goes ahead anyway.
The first approver can avoid it by approving last. Under the current code, match wallets receive 0 ETH from LikeRegistry, so only ETH that the owners deposit themselves through receive() is exposed.

Impact:

The first approver cannot withdraw consent. In the PoC, Bob executes a 1 ETH transfer to himself 365 days after Alice approved it, on that year-old approval.
An approved transaction that failed for lack of funds stays pending. It executes once the wallet is funded for another purpose: in the PoC, the approved 1 ETH moves out of a later 5 ETH deposit. The 2-of-2 rule is never broken, and the amount never exceeds what was approved.

Proof of Concept

The PoC deploys MultiSigWallet(alice, bob) directly. The baseline shows that one approval alone cannot execute (Not enough approvals). The other tests show the following:

No revoke or cancel: four common revoke or cancel selectors all revert, because the contract has no such function and no fallback. Re-approving reverts AlreadyApproved, and the flag stays set.
No expiry: Bob submits a 1 ETH transfer to himself and Alice approves it. 365 days later, Bob alone approves and executes it and receives 1 ETH.
Unfunded transaction: an approved transaction that failed for lack of funds stays pending. 180 days later Alice deposits 5 ETH for another purpose. Bob then executes the transaction: exactly the approved 1 ETH moves and 4 ETH stay in the wallet.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import {Test, console} from "forge-std/Test.sol";

import {MultiSigWallet} from "../src/MultiSig.sol";

contract PoC_DD12 is Test {

MultiSigWallet wallet;

address alice = makeAddr("alice"); // owner1 (victim, approves first)

address bob = makeAddr("bob"); // owner2 (counterparty)

function setUp() public {

wallet = new MultiSigWallet(alice, bob);

vm.deal(alice, 10 ether);

vm.deal(bob, 10 ether);

}

function _flags(uint256 id) internal view returns (bool a1, bool a2, bool ex) {

(,, a1, a2, ex) = wallet.transactions(id);

}

/// Baseline: one approval alone is not enough, the 2-of-2 gate works.

function test_Baseline_SingleApprovalCannotExecute() public {

vm.prank(alice);

(bool ok,) = address(wallet).call{value: 1 ether}("");

assertTrue(ok);

vm.prank(bob);

wallet.submitTransaction(bob, 1 ether);

vm.prank(alice);

wallet.approveTransaction(0);

vm.prank(bob);

vm.expectRevert(bytes("Not enough approvals"));

wallet.executeTransaction(0);

}

/// Limb 1: an approval can never be withdrawn and a tx can never be cancelled.

function test_ApprovalIsIrrevocable() public {

vm.prank(bob);

wallet.submitTransaction(bob, 1 ether);

vm.prank(alice);

wallet.approveTransaction(0);

// Common revoke / cancel entry points do not exist (no fallback -> revert).

bytes4[4] memory sels = [

bytes4(keccak256("revokeApproval(uint256)")),

bytes4(keccak256("revokeConfirmation(uint256)")),

bytes4(keccak256("cancelTransaction(uint256)")),

bytes4(keccak256("unapproveTransaction(uint256)"))

];

for (uint256 i; i < sels.length; i++) {

vm.prank(alice);

(bool ok,) = address(wallet).call(abi.encodeWithSelector(sels[i], uint256(0)));

assertFalse(ok, "unexpected revoke/cancel function exists");

}

// Re-approving does not toggle: it reverts and the flag stays true.

vm.prank(alice);

vm.expectRevert(MultiSigWallet.AlreadyApproved.selector);

wallet.approveTransaction(0);

(bool a1, bool a2, bool ex) = _flags(0);

assertTrue(a1, "alice approval still set");

assertFalse(a2);

assertFalse(ex);

}

/// Limb 2: no expiry. One year later, Bob alone completes and executes the tx

/// on Alice's year-old approval.

function test_StaleApprovalExecutesAfterOneYear() public {

vm.prank(alice);

(bool ok,) = address(wallet).call{value: 1 ether}("");

assertTrue(ok);

vm.prank(bob);

wallet.submitTransaction(bob, 1 ether);

vm.prank(alice);

wallet.approveTransaction(0);

// Plans change. Alice has no way to withdraw consent.

vm.warp(block.timestamp + 365 days);

vm.roll(block.number + 2_600_000);

uint256 bobBefore = bob.balance;

vm.startPrank(bob);

wallet.approveTransaction(0);

wallet.executeTransaction(0);

vm.stopPrank();

console.log("bob gained (wei)", bob.balance - bobBefore);

assertEq(bob.balance - bobBefore, 1 ether, "stale tx executed for Bob");

assertEq(address(wallet).balance, 0);

(,,,, bool ex) = wallet.transactions(0);

assertTrue(ex);

}

/// Correction to the claim: the amount is fixed at submission. Balance growth

/// never increases what a stale tx moves; it can only make an approved but

/// unfunded tx executable later.

function test_AmountFixed_UnfundedApprovedTxBecomesExecutableLater() public {

vm.prank(bob);

wallet.submitTransaction(bob, 1 ether);

vm.prank(alice);

wallet.approveTransaction(0);

vm.prank(bob);

wallet.approveTransaction(0);

// Unfunded: execution fails now (and rolls back, tx stays pending).

vm.prank(bob);

vm.expectRevert(bytes("Transaction failed"));

wallet.executeTransaction(0);

(,,,, bool ex0) = wallet.transactions(0);

assertFalse(ex0, "still pending after failed execution");

// Months later Alice funds the wallet for a different purpose (5 ETH).

vm.warp(block.timestamp + 180 days);

vm.prank(alice);

(bool ok,) = address(wallet).call{value: 5 ether}("");

assertTrue(ok);

uint256 bobBefore = bob.balance;

vm.prank(bob);

wallet.executeTransaction(0);

console.log("wallet balance after (wei)", address(wallet).balance);

assertEq(bob.balance - bobBefore, 1 ether, "only the approved value moves");

assertEq(address(wallet).balance, 4 ether, "rest of balance untouched");

}

Run from the repository root, with the file saved as test/PoC_DD-12.t.sol:

FOUNDRY_FFI=false FOUNDRY_DISABLE_NIGHTLY_WARNING=1 forge test --match-path test/PoC_DD-12.t.sol -vv

Result: Suite result: ok. 4 passed; 0 failed; 0 skipped. The logs show bob gained (wei) 1000000000000000000 and wallet balance after (wei) 4000000000000000000.

Recommended Mitigation

Let an owner withdraw an approval before execution. Optionally, also add a deadline to Transaction and check it in executeTransaction.

--- a/src/MultiSig.sol

+++ b/src/MultiSig.sol

@@ -24,6 +24,7 @@

event TransactionCreated(uint256 indexed txId, address indexed to, uint256 value);

event TransactionApproved(uint256 indexed txId, address indexed owner);

event TransactionExecuted(uint256 indexed txId, address indexed to, uint256 value);

+ event ApprovalRevoked(uint256 indexed txId, address indexed owner);

modifier onlyOwners() {

if (msg.sender != owner1 && msg.sender != owner2) revert NotAnOwner();

@@ -64,6 +65,21 @@

emit TransactionApproved(_txId, msg.sender);

}

+ /// @notice Withdraw an approval before the transaction is executed

+ function revokeApproval(uint256 _txId) external onlyOwners {

+ require(_txId < transactions.length, "Invalid transaction ID");

+ Transaction storage txn = transactions[_txId];

+ require(!txn.executed, "Transaction already executed");

+ if (msg.sender == owner1) {

+ txn.approvedByOwner1 = false;

+ } else {

+ txn.approvedByOwner2 = false;

+ }

+ emit ApprovalRevoked(_txId, msg.sender);

+ }

/// @notice Execute a fully-approved transaction

function executeTransaction(uint256 _txId) external onlyOwners {

require(_txId < transactions.length, "Invalid transaction ID");

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour ago

Submission Judgement Published

Invalidated

Reason: Incorrect statement

Rewards breakdown

This contest has a flat reward structure with the following payouts:

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being reviewed by our AI judge. Results will be available in a few minutes.

View all submissions

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

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

Give us feedback!