From 8beb38a6e166e215aeaeee854e63fc2442cd6572 Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 4 Oct 2023 01:43:59 +0000 Subject: [PATCH 1/4] Added msg.sender check to withdraw method --- .../facets/AccountsDepositWithdrawEndowments.sol | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol index 6e2057ba2..60e08421d 100644 --- a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol +++ b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol @@ -255,6 +255,10 @@ contract AccountsDepositWithdrawEndowments is beneficiaryEndowId = tempEndowmentState.closingBeneficiary.data.endowId; } } + // If not closing, only owner can call + else { + require(msg.sender == tempEndowment.owner, "Unauthorized"); + } // place an arbitrary cap on the qty of different tokens per withdraw to limit gas use require(tokens.length > 0, "No tokens provided"); @@ -273,10 +277,6 @@ contract AccountsDepositWithdrawEndowments is ); } - // Check if maturity has been reached for the endowment (0 == no maturity date) - bool mature = (tempEndowment.maturityTime != 0 && - block.timestamp >= tempEndowment.maturityTime); - if (tempEndowment.endowType == LibAccounts.EndowmentType.Daf) { require( beneficiaryAddress == address(0), @@ -307,6 +307,10 @@ contract AccountsDepositWithdrawEndowments is ); } + // Check if maturity has been reached for the endowment (0 == no maturity date) + bool mature = (tempEndowment.maturityTime != 0 && + block.timestamp >= tempEndowment.maturityTime); + for (uint256 t = 0; t < tokens.length; t++) { // ensure balance of tokens can cover the requested withdraw amount require(state.Balances[id][acctType][tokens[t].addr] >= tokens[t].amnt, "Insufficient Funds"); From 6514c353ef0f69768d9311233a95b366daf6a4ad Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 4 Oct 2023 01:45:45 +0000 Subject: [PATCH 2/4] Move maturity check back to original line --- .../accounts/facets/AccountsDepositWithdrawEndowments.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol index 60e08421d..f9a161585 100644 --- a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol +++ b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol @@ -277,6 +277,10 @@ contract AccountsDepositWithdrawEndowments is ); } + // Check if maturity has been reached for the endowment (0 == no maturity date) + bool mature = (tempEndowment.maturityTime != 0 && + block.timestamp >= tempEndowment.maturityTime); + if (tempEndowment.endowType == LibAccounts.EndowmentType.Daf) { require( beneficiaryAddress == address(0), @@ -307,10 +311,6 @@ contract AccountsDepositWithdrawEndowments is ); } - // Check if maturity has been reached for the endowment (0 == no maturity date) - bool mature = (tempEndowment.maturityTime != 0 && - block.timestamp >= tempEndowment.maturityTime); - for (uint256 t = 0; t < tokens.length; t++) { // ensure balance of tokens can cover the requested withdraw amount require(state.Balances[id][acctType][tokens[t].addr] >= tokens[t].amnt, "Insufficient Funds"); From 90bc940e70d522aff9bea3bf2994e04b4ce3f9b9 Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 4 Oct 2023 02:44:25 +0000 Subject: [PATCH 3/4] Add maturity check and adjust tests to expect appropriate senders --- .../AccountsDepositWithdrawEndowments.sol | 25 ++++++++++++++----- .../AccountsDepositWithdrawEndowments.ts | 25 +++++++++++++------ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol index f9a161585..6e38f247b 100644 --- a/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol +++ b/contracts/core/accounts/facets/AccountsDepositWithdrawEndowments.sol @@ -234,6 +234,10 @@ contract AccountsDepositWithdrawEndowments is AccountStorage.Endowment storage tempEndowment = state.Endowments[id]; AccountStorage.EndowmentState storage tempEndowmentState = state.States[id]; + // Check if maturity has been reached for the endowment (0 == no maturity date) + bool mature = (tempEndowment.maturityTime != 0 && + block.timestamp >= tempEndowment.maturityTime); + // If an Endowment is closed and a withdraw is executed for it, check that the sender is the closing // beneficiary on record and procees the withdraw if so ensure funds are indeed moving to that beneficiary if (tempEndowmentState.closingEndowment) { @@ -255,9 +259,22 @@ contract AccountsDepositWithdrawEndowments is beneficiaryEndowId = tempEndowmentState.closingBeneficiary.data.endowId; } } - // If not closing, only owner can call + // If not closing, else { - require(msg.sender == tempEndowment.owner, "Unauthorized"); + // only owner can call if not mature + if (!mature) { + require(msg.sender == tempEndowment.owner, "Unauthorized"); + } + // otherwise the caller must be in the Allowlist + else { + require( + IterableMappingAddr.get( + state.Allowlists[id][LibAccounts.AllowlistType.MaturityAllowlist], + msg.sender + ), + "Unauthorized" + ); + } } // place an arbitrary cap on the qty of different tokens per withdraw to limit gas use @@ -277,10 +294,6 @@ contract AccountsDepositWithdrawEndowments is ); } - // Check if maturity has been reached for the endowment (0 == no maturity date) - bool mature = (tempEndowment.maturityTime != 0 && - block.timestamp >= tempEndowment.maturityTime); - if (tempEndowment.endowType == LibAccounts.EndowmentType.Daf) { require( beneficiaryAddress == address(0), diff --git a/test/core/accounts/AccountsDepositWithdrawEndowments.ts b/test/core/accounts/AccountsDepositWithdrawEndowments.ts index 0c957964c..316732c65 100644 --- a/test/core/accounts/AccountsDepositWithdrawEndowments.ts +++ b/test/core/accounts/AccountsDepositWithdrawEndowments.ts @@ -18,6 +18,7 @@ import { IAccountsDepositWithdrawEndowments, IERC20, IERC20__factory, + OwnershipFacet__factory, Registrar, Registrar__factory, TestFacetProxyContract, @@ -1099,7 +1100,9 @@ describe("AccountsDepositWithdrawEndowments", function () { let finalAmountLeftover = amountLeftAfterApFees.sub(expectedFeeEndow); await expect( - facet.withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) + facet + .connect(endowOwner) + .withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) ) .to.emit(facet, "EndowmentWithdraw") .withArgs( @@ -1145,7 +1148,9 @@ describe("AccountsDepositWithdrawEndowments", function () { await wait(state.setAllowlist(normalEndowId, 0, [beneficiaryAddress])); await expect( - facet.withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) + facet + .connect(endowOwner) + .withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) ) .to.emit(facet, "EndowmentWithdraw") .withArgs( @@ -1323,7 +1328,7 @@ describe("AccountsDepositWithdrawEndowments", function () { await expect( facet.withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) - ).to.be.revertedWith("Beneficiary address is not listed in maturityAllowlist"); + ).to.be.revertedWith("Unauthorized"); }); describe("LOCKED withdrawals", () => { @@ -1337,7 +1342,13 @@ describe("AccountsDepositWithdrawEndowments", function () { maturityTime: currTime, }; await wait(state.setEndowmentDetails(normalEndowId, matureEndowment)); - await wait(state.setAllowlist(normalEndowId, 2, [beneficiaryAddress])); + const beneficiarySigner = new ethers.Wallet( + genWallet().privateKey, + hre.ethers.provider + ); + await accOwner.sendTransaction({value: ethers.utils.parseEther("1"), to: beneficiarySigner.address}) + + await wait(state.setAllowlist(normalEndowId, 2, [beneficiarySigner.address])); const acctType = VaultType.LOCKED; const beneficiaryId = 0; @@ -1349,7 +1360,7 @@ describe("AccountsDepositWithdrawEndowments", function () { const finalAmountLeftover = amount.sub(expectedFeeAp); await expect( - facet.withdraw(normalEndowId, acctType, beneficiaryAddress, beneficiaryId, tokens) + facet.connect(beneficiarySigner).withdraw(normalEndowId, acctType, beneficiarySigner.address, beneficiaryId, tokens) ) .to.emit(facet, "EndowmentWithdraw") .withArgs( @@ -1357,13 +1368,13 @@ describe("AccountsDepositWithdrawEndowments", function () { tokens[0].addr, tokens[0].amnt, acctType, - beneficiaryAddress, + beneficiarySigner.address, beneficiaryId ); expect(tokenFake.transfer).to.have.been.calledWith(treasury, expectedFeeAp); expect(tokenFake.transfer).to.have.been.calledWith( - beneficiaryAddress, + beneficiarySigner.address, finalAmountLeftover ); From 795d3b2d05324e8cf984ed9f8bdd7443c70e78b1 Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 4 Oct 2023 02:55:42 +0000 Subject: [PATCH 4/4] Remove accidental import --- test/core/accounts/AccountsDepositWithdrawEndowments.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/core/accounts/AccountsDepositWithdrawEndowments.ts b/test/core/accounts/AccountsDepositWithdrawEndowments.ts index 316732c65..34d404636 100644 --- a/test/core/accounts/AccountsDepositWithdrawEndowments.ts +++ b/test/core/accounts/AccountsDepositWithdrawEndowments.ts @@ -18,7 +18,6 @@ import { IAccountsDepositWithdrawEndowments, IERC20, IERC20__factory, - OwnershipFacet__factory, Registrar, Registrar__factory, TestFacetProxyContract,