Unsign Tx and Delete PendingTx after Completion

src/Multisig.sol

@@ -16,6 +16,7 @@ contract Multisig {
     );
     event SignTx(bytes32 pendingHash, address signer);
     event UnsignTx(bytes32 pendingHash, address signer);
+    event RemoveTx(bytes32 pendingHash);
     event SendTx(bytes32 pendingHash);
     event AddOwner(address newOwner);
     event RemoveOwner(address owner);
@@ -174,26 +175,43 @@ contract Multisig {
     /// @notice Signs off on a transaction and execute it if enough signatures
     /// @param pendingHash Hash that maps to the PendingTx to unsign
     function signTx(bytes32 pendingHash) public onlyOwner {
-        // sign tx
-        // log event
-        // sendTx if enough sigs
         PendingTx storage pendingTx = pending[pendingHash];
+
         require(pendingTx.nSigned > 0, "Transaction does not exist!");
         require(!pendingTx.signers[msg.sender], "Already signed!");
+
         pendingTx.signers[msg.sender] = true;
         pendingTx.nSigned += 1;
+
         if (pending[pendingHash].nSigned >= nNeeded) {
-            sendTx(pendingHash);
             emit SignTx(pendingHash, msg.sender);
+            sendTx(pendingHash);
+            // Doesn't delete signers mapping
+            // https://docs.soliditylang.org/en/develop/types.html#delete
+            // TODO: Keep track of signers in an Array as well?
+            delete pending[pendingHash];
         }
     }
 
     /// @notice Removes existing signature from a PendingTx
     /// @dev Check if they're signature exists to properly update nSigned
     /// @param pendingHash Hash that maps to the PendingTx to unsign
     function unsignTx(bytes32 pendingHash) public onlyOwner {
-        // remove signature from tx
-        // log event
+        PendingTx storage pendingTx = pending[pendingHash];
+
+        require(pendingTx.nSigned > 0, "Transaction does not exist!");
+
+        if (pendingTx.signers[msg.sender]) {
+            pendingTx.signers[msg.sender] = false;
+            pendingTx.nSigned -= 1;
+            emit UnsignTx(pendingHash, msg.sender);
+        }
+
+        if (pendingTx.nSigned == 0) {
+            // Dead proposed transaction, delete it.
+            emit RemoveTx(pendingHash);
+            delete pending[pendingHash];
+        }
     }
 
     /// @notice Wrapper to send transaction once approved

src/test/Multisig.sol

@@ -198,33 +198,104 @@ contract MultisigTest is DSTest {
         vm.deal(address(owner1), 50000);
         vm.deal(address(owner2), 50000);
 
+        /// Test that signTx removed pending tx after completion
+
         vm.prank(owner1);
         bytes32 pendingHashObs = multisig.createTx(to, value, data);
 
         // try sign with another owner
         vm.prank(owner2);
         multisig.signTx(pendingHashObs);
 
-        // check pending hash signers, nSigned
+        // check that pending hash has been
+        // removed after all signers have signed.
         bytes32 txHashObs;
         uint256 nSignedObs;
         (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
-        assertEq(nSignedObs, 2, "incorrect nSignedObs");
+        assertEq(nSignedObs, 0, "pending tx should be deleted");
+
+        /// Test that signTx correctly adds signer
+        address newOwner = address(0x456);
+
+        // Change numNeeded so pending tx is not deleted after 2 signers.
+        vm.prank(address(multisig));
+        multisig.addOwner(newOwner);
+        vm.prank(address(multisig));
+        multisig.changeNumNeeded(3);
+
+        vm.prank(owner1);
+        // TODO: this is submitting the same tx as above, but once sent,
+        // the signers from the previous pending tx are not removed since
+        // the keys are not known. So adding owner2 will result in
+        // Already Signed error. See TODO comment in signTx.
+        pendingHashObs = multisig.createTx(to, value, data);
+
+        // try sign with another owner
+        // vm.prank(owner2); See above comment.
+        vm.prank(newOwner);
+        multisig.signTx(pendingHashObs);
+
+        // check that pending hash has corrent num signers
+        (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
+        assertEq(nSignedObs, 2, "incorrect num signers");
 
         uint256 result = 0; // No assertEq for bools
-        if (multisig.getSigner(pendingHashObs, owner2)) {
+        if (multisig.getSigner(pendingHashObs, newOwner)) {
             result = 1;
         }
         assertEq(result, 1, "signTx did not add signer");
 
         // sign with already signed owner, check nSigned
-        vm.prank(owner2);
+        vm.prank(owner1);
         vm.expectRevert("Already signed!");
         multisig.signTx(pendingHashObs);
         (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
         assertEq(nSignedObs, 2, "incorrect nSignedObs");
     }
 
+    function testUnsignTx() public {
+        // createTx
+        address to = address(0x123);
+        uint256 value = 1;
+        bytes memory data = abi.encode("asdf");
+
+        vm.deal(address(multisig), 50000);
+        vm.deal(address(owner1), 50000);
+        vm.deal(address(owner2), 50000);
+
+        vm.prank(owner1);
+        bytes32 pendingHashObs = multisig.createTx(to, value, data);
+
+        // unsign after create tx should delete pending tx
+        vm.prank(owner1);
+        multisig.unsignTx(pendingHashObs);
+        bytes32 txHashObs;
+        uint256 nSignedObs;
+        (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
+        uint256 result = 0;
+        if (nSignedObs > 0) {
+            result = 1;
+        }
+        assertEq(result, 0, "pendingHashObs still exists");
+
+        vm.prank(owner1);
+        pendingHashObs = multisig.createTx(to, value, data);
+
+        // unsign with owner2
+        vm.prank(owner2);
+        multisig.unsignTx(pendingHashObs);
+
+        // check pending hash signers, nSigned
+        (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
+        assertEq(nSignedObs, 1, "incorrect nSignedObs");
+
+        result = 0; // No assertEq for bools
+        if (multisig.getSigner(pendingHashObs, owner2)) {
+            result = 1;
+        }
+        assertEq(result, 0, "unsignTx did not remove signer");
+    }
+
     function testSignAndSendTx() public {
         // sending eth
         address to = owner1;
@@ -332,28 +403,30 @@ contract MultisigTest is DSTest {
         vm.prank(owner1);
         bytes32 pendingHashObs = multisig.createTx(to, value, data);
 
-        // try sign with another owner
-        vm.prank(owner2);
+        // sign with already signed owner, check nSigned
+        vm.prank(owner1);
+        vm.expectRevert("Already signed!");
         multisig.signTx(pendingHashObs);
-
-        // check pending hash signers, nSigned
         bytes32 txHashObs;
         uint256 nSignedObs;
         (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
-        assertEq(nSignedObs, 2, "incorrect nSignedObs");
+        assertEq(nSignedObs, 1, "incorrect nSignedObs");
 
         uint256 result = 0; // No assertEq for bools
         if (multisig.getSigner(pendingHashObs, owner1)) {
             result = 1;
         }
         assertEq(result, 1, "signTx did not add signer");
 
-        // sign with already signed owner, check nSigned
+        // try sign with another owner
         vm.prank(owner2);
         vm.expectRevert("Already signed!");
         multisig.signTx(pendingHashObs);
-        (txHashObs, nSignedObs) = multisig.pending(pendingHashObs);
-        assertEq(nSignedObs, 2, "incorrect nSignedObs");
+
+        // Check that pending has been removed
+        vm.prank(owner1);
+        vm.expectRevert("Transaction does not exist!");
+        multisig.signTx(pendingHashObs);
     }
 
     function testSignAndSendTxWithFuzzing(