fix(vault): do no allow reuse of fund ids (#238)

* fix(vault): do no allow reuse of fund ids

Fixes an attack where all tokens can be drained from
the Vault by allowing a token flow to persist after
a Fund is deleted.

* chore(vault): update state diagram

contracts/vault/Funds.sol

@@ -10,22 +10,18 @@ struct Fund {
   Timestamp lockMaximum;
   /// Indicates whether fund is frozen, and at what time
   Timestamp frozenAt;
-  /// The total amount of tokens in the fund
-  uint128 value;
 }
 
 /// A fund can go through the following states:
 ///
-///     -----------------------------------------------
+///     -->  Inactive ---> Locked -----> Withdrawing
-///    |                                               |
-///     -->  Inactive ---> Locked -----> Withdrawing --
 ///                          \               ^
 ///                           \             /
 ///                            --> Frozen --
 ///
 enum FundStatus {
   /// Indicates that the fund is inactive and contains no tokens. This is the
-  /// initial state, or the state after all tokens have been withdrawn.
+  /// initial state.
   Inactive,
   /// Indicates that a time-lock is set and withdrawing tokens is not allowed. A
   /// fund needs to be locked for deposits, transfers, flows and burning to be

contracts/vault/VaultBase.sol

@@ -132,7 +132,6 @@ abstract contract VaultBase {
     Account storage account = _accounts[controller][fundId][accountId];
 
     account.balance.available += amount;
-    fund.value += amount;
 
     _token.safeTransferFrom(
       Controller.unwrap(controller),
@@ -215,8 +214,6 @@ abstract contract VaultBase {
 
     account.balance.designated -= amount;
 
-    fund.value -= amount;
-
     _token.safeTransfer(address(0xdead), amount);
   }
 
@@ -232,8 +229,6 @@ abstract contract VaultBase {
     require(account.flow.incoming == account.flow.outgoing, VaultFlowNotZero());
     uint128 amount = account.balance.available + account.balance.designated;
 
-    fund.value -= amount;
-
     delete _accounts[controller][fundId][accountId];
 
     _token.safeTransfer(address(0xdead), amount);
@@ -258,14 +253,6 @@ abstract contract VaultBase {
     account.accumulateFlows(fund.flowEnd());
     uint128 amount = account.balance.available + account.balance.designated;
 
-    fund.value -= amount;
-
-    if (fund.value == 0) {
-      delete _funds[controller][fundId];
-    } else {
-      _funds[controller][fundId] = fund;
-    }
-
     delete _accounts[controller][fundId][accountId];
 
     (address owner, ) = Accounts.decodeId(accountId);

test/Vault.tests.js

@@ -693,27 +693,13 @@ describe("Vault", function () {
         await expect(locking).to.be.revertedWith("AlreadyLocked")
       })
 
-      it("deletes lock when no tokens remain", async function () {
+      it("cannot set lock when no tokens remain", async function () {
         await token.connect(controller).approve(vault.address, 30)
         await vault.deposit(fund, account1, 30)
-        await vault.transfer(fund, account1, account2, 20)
-        await vault.transfer(fund, account2, account3, 10)
-        // some designated tokens are burned
-        await vault.designate(fund, account2, 10)
-        await vault.burnDesignated(fund, account2, 5)
-        // some holder is burned
-        await vault.burnAccount(fund, account2)
         await expire()
-        // some tokens are withdrawn
         await vault.withdraw(fund, account1)
-        expect(await vault.getFundStatus(fund)).to.equal(FundStatus.Withdrawing)
+        const locking = vault.lock(fund, expiry, maximum)
-        expect(await vault.getLockExpiry(fund)).not.to.equal(0)
+        await expect(locking).to.be.revertedWith("AlreadyLocked")
-        // remainder of the tokens are withdrawn by recipient
-        await vault
-          .connect(holder3)
-          .withdrawByRecipient(controller.address, fund, account3)
-        expect(await vault.getFundStatus(fund)).to.equal(FundStatus.Inactive)
-        expect(await vault.getLockExpiry(fund)).to.equal(0)
       })
     })
 
@@ -1142,4 +1128,46 @@ describe("Vault", function () {
       })
     })
   })
+
+  describe("bugs", function () {
+    it("does not allow flows to survive fund id reuse", async function () {
+      // bug discovered and reported by Aleksander and Jochen from Certora
+      async function reproduceBug() {
+        const account1 = await vault.encodeAccountId(
+          holder.address,
+          randomBytes(12)
+        )
+        const account2 = await vault.encodeAccountId(
+          holder.address,
+          randomBytes(12)
+        )
+        const expiry1 = (await currentTime()) + 10
+        const expiry2 = (await currentTime()) + 20
+
+        // store tokens in fund
+        await token.connect(controller).approve(vault.address, 100)
+        await vault.lock(fund, expiry1, expiry1)
+        await vault.deposit(fund, account1, 100)
+
+        // initiate a flow, and immediately freeze it
+        await vault.flow(fund, account1, account2, 1)
+        await vault.freezeFund(fund)
+
+        // only withdraw from flow sender
+        await advanceTimeTo(expiry1)
+        expect(await vault.getBalance(fund, account1)).to.equal(100)
+        await vault.withdraw(fund, account1)
+
+        // reuse fund id
+        await vault.lock(fund, expiry2, expiry2)
+        await advanceTimeTo(expiry2)
+
+        // bug: this balance is positive, because the flow was not reset
+        expect(await vault.getBalance(fund, account2)).to.equal(20)
+      }
+
+      // bug is fixed by no longer allowing reuse of fund ids
+      await expect(reproduceBug()).to.be.revertedWith("VaultFundAlreadyLocked")
+    })
+  })
 })