From 470a4df4152b53a8065c2984d55efcd7350b549a Mon Sep 17 00:00:00 2001 From: markspanbroek Date: Mon, 19 May 2025 12:23:01 +0200 Subject: [PATCH] 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 | 8 ++--- contracts/vault/VaultBase.sol | 13 -------- test/Vault.tests.js | 62 +++++++++++++++++++++++++---------- 3 files changed, 47 insertions(+), 36 deletions(-) diff --git a/contracts/vault/Funds.sol b/contracts/vault/Funds.sol index 06bbfa8..8b69c35 100644 --- a/contracts/vault/Funds.sol +++ b/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 diff --git a/contracts/vault/VaultBase.sol b/contracts/vault/VaultBase.sol index 06df5b5..9e9622f 100644 --- a/contracts/vault/VaultBase.sol +++ b/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); diff --git a/test/Vault.tests.js b/test/Vault.tests.js index 5043ddf..f91c612 100644 --- a/test/Vault.tests.js +++ b/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) - expect(await vault.getLockExpiry(fund)).not.to.equal(0) - // 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) + const locking = vault.lock(fund, expiry, maximum) + await expect(locking).to.be.revertedWith("AlreadyLocked") }) }) @@ -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") + }) + }) })