From 9c16ad4aa9e8f7df702c9735c175b347565b4246 Mon Sep 17 00:00:00 2001 From: realdiganta <47485188+dxganta@users.noreply.github.com> Date: Tue, 10 Jan 2023 19:24:54 +0530 Subject: [PATCH 1/3] fix to dust issue on unforced withdrawals under special case --- contracts/Vault.sol | 22 ++++++++++++---------- test/vault/Vault.spec.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/contracts/Vault.sol b/contracts/Vault.sol index 6087c953..62f92a3c 100644 --- a/contracts/Vault.sol +++ b/contracts/Vault.sol @@ -1101,7 +1101,7 @@ contract Vault is revert VaultCannotWithdrawMoreThanAvailable(); // Amount of shares the _amount is worth - uint256 amountShares = _computeShares( + uint256 sharesToBurn = _computeShares( _amount, _totalShares, _totalUnderlyingMinusSponsored @@ -1112,13 +1112,12 @@ contract Vault is uint256 claimerShares = (_amount * _claim.totalShares) / _claim.totalPrincipal; - if (!_force && amountShares > claimerShares) + if (!_force && sharesToBurn > claimerShares) revert VaultMustUseForceWithdrawToAcceptLosses(); - uint256 sharesToBurn = amountShares; + bool haircut = sharesToBurn > claimerShares; - if (_force && amountShares > claimerShares) - sharesToBurn = claimerShares; + if (haircut) sharesToBurn = claimerShares; claimer[_deposit.claimerId].totalShares -= sharesToBurn; claimer[_deposit.claimerId].totalPrincipal -= _amount; @@ -1134,11 +1133,14 @@ contract Vault is deposits[_tokenId].amount -= _amount; } - uint256 amount = _computeAmount( - sharesToBurn, - _totalShares, - _totalUnderlyingMinusSponsored - ); + uint256 amount = _amount; + if (haircut) { + amount = _computeAmount( + sharesToBurn, + _totalShares, + _totalUnderlyingMinusSponsored + ); + } emit DepositWithdrawn(_tokenId, sharesToBurn, amount, _to, isFull); diff --git a/test/vault/Vault.spec.ts b/test/vault/Vault.spec.ts index 2aa1ad31..85351848 100644 --- a/test/vault/Vault.spec.ts +++ b/test/vault/Vault.spec.ts @@ -2949,6 +2949,33 @@ describe('Vault', () => { await vault.connect(admin).exitUnpause(); }); + + it('leaves no dust on withdraw when force is false', async () => { + const toDeposit = parseUnits('100'); + const prevBal = await underlying.balanceOf(alice.address); + + const params = depositParams.build({ + amount: toDeposit, + inputToken: underlying.address, + claims: [ + claimParams.percent(50).to(alice.address).build(), + claimParams.percent(50).to(bob.address).build(), + ], + }); + + await vault.connect(alice).deposit(params); + + await moveForwardTwoWeeks(); + await addYieldToVault('100'); + + await vault + .connect(alice) + .partialWithdraw(alice.address, [2], [parseUnits('1')]); + + const newBal = await underlying.balanceOf(alice.address); + + expect(prevBal.sub(newBal)).to.eq(toDeposit.sub(parseUnits('1'))); + }); }); describe('claimYield', () => { From 2206d3e38d54eabcb3f34d07107844bde05e6e64 Mon Sep 17 00:00:00 2001 From: realdiganta <47485188+dxganta@users.noreply.github.com> Date: Wed, 11 Jan 2023 12:18:55 +0530 Subject: [PATCH 2/3] dust allignments removed from test --- test/audit_2.spec.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/audit_2.spec.ts b/test/audit_2.spec.ts index 7eccb4ac..6eb93b19 100644 --- a/test/audit_2.spec.ts +++ b/test/audit_2.spec.ts @@ -152,12 +152,10 @@ describe('Audit Tests 2', () => { await vault.connect(charlie).withdraw(charlie.address, [5]); - expect(await underlying.balanceOf(charlie.address)).to.eq( - oldBalance.sub(1), - ); + expect(await underlying.balanceOf(charlie.address)).to.eq(oldBalance); }); - it('price per share can be manipulated', async () => { + it('price per share can not be manipulated', async () => { await addUnderlyingBalance(alice, '10000'); await underlying.mint(bob.address, parseUnits('10000')); await underlying.mint(charlie.address, parseUnits('10000')); @@ -285,7 +283,7 @@ describe('Audit Tests 2', () => { await vault.connect(charlie).withdraw(charlie.address, [4]); - expect(Number(await underlying.balanceOf(charlie.address))).to.lessThan( + expect(Number(await underlying.balanceOf(charlie.address))).to.equal( oldBalance, ); }); From d26ffa0dad7bec090b6cbee3ff3ebd3744549a53 Mon Sep 17 00:00:00 2001 From: realdiganta <47485188+dxganta@users.noreply.github.com> Date: Wed, 11 Jan 2023 13:38:15 +0530 Subject: [PATCH 3/3] fixed tests failing to dust accountancy in the tests --- test/audit_3.spec.ts | 4 ++-- .../strategy/yearn/YearnStrategy.fork.spec.ts | 8 ++++--- test/vault/VaultSyncMode.spec.ts | 23 +++++++++---------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/test/audit_3.spec.ts b/test/audit_3.spec.ts index 47253250..b9454360 100644 --- a/test/audit_3.spec.ts +++ b/test/audit_3.spec.ts @@ -201,7 +201,7 @@ describe('Audit Tests 3', () => { // expect depositor3 can normally withdraw await vault.connect(depositor3).withdraw(depositor3.address, [3]); expect(await underlying.balanceOf(depositor3.address)).to.eq( - parseUnits('100000').sub(1), + parseUnits('100000'), ); console.log( 'depositor3 underlying balance: ' + @@ -293,7 +293,7 @@ describe('Audit Tests 3', () => { // expect depositor3 can normally withdraw but must use forceWithdraw await vault.connect(depositor3).forceWithdraw(depositor3.address, [3]); expect(await underlying.balanceOf(depositor3.address)).to.eq( - parseUnits('100000').sub(1), + parseUnits('100000'), ); console.log( 'depositor3 underlying balance: ' + diff --git a/test/strategy/yearn/YearnStrategy.fork.spec.ts b/test/strategy/yearn/YearnStrategy.fork.spec.ts index 60b5d091..31931d81 100644 --- a/test/strategy/yearn/YearnStrategy.fork.spec.ts +++ b/test/strategy/yearn/YearnStrategy.fork.spec.ts @@ -125,9 +125,11 @@ describe('Yearn Strategy (mainnet fork tests)', () => { parseUnits('1000', await lusd.decimals()), ); + const depositAmt = parseUnits('1000'); + await vault.connect(alice).deposit( depositParams.build({ - amount: parseUnits('1000'), + amount: depositAmt, inputToken: lusd.address, claims: [claimParams.percent(100).to(alice.address).build()], }), @@ -150,7 +152,7 @@ describe('Yearn Strategy (mainnet fork tests)', () => { await vault.connect(alice).withdraw(alice.address, [1]); const aliceBalance = await lusd.balanceOf(alice.address); - expect(aliceBalance).to.eq('999999999999999999999'); + expect(aliceBalance).to.eq(depositAmt); }); it('allows user to claim yield when Yearn Vault performs', async () => { @@ -184,7 +186,7 @@ describe('Yearn Strategy (mainnet fork tests)', () => { await vault.connect(alice).withdraw(alice.address, [1]); const aliceBalance = await lusd.balanceOf(alice.address); - expect(aliceBalance).to.eq('1089999999999999999174'); + expect(aliceBalance).to.eq('1089999999999999999175'); }); it('allows user to only do force withdrawal when Yearn Vault underperforms', async () => { diff --git a/test/vault/VaultSyncMode.spec.ts b/test/vault/VaultSyncMode.spec.ts index 87cfebd3..e7bcd282 100644 --- a/test/vault/VaultSyncMode.spec.ts +++ b/test/vault/VaultSyncMode.spec.ts @@ -245,7 +245,7 @@ describe('Vault in sync mode', () => { await vault.connect(alice).withdraw(alice.address, [1]); expect(await underlying.balanceOf(alice.address)).to.eq( - '999999999999999999999', + parseUnits('1000'), ); }); @@ -265,7 +265,7 @@ describe('Vault in sync mode', () => { await vault.connect(alice).withdraw(alice.address, [1]); expect(await underlying.balanceOf(alice.address)).to.eq( - '999999999999999999999', + parseUnits('1000'), ); }); @@ -285,7 +285,7 @@ describe('Vault in sync mode', () => { expect(await underlying.balanceOf(vault.address)).to.eq(parseUnits('5')); expect(await underlying.balanceOf(strategy.address)).to.eq( - '45000000000000000001', + parseUnits('45'), ); }); @@ -332,7 +332,8 @@ describe('Vault in sync mode', () => { await moveForwardTwoWeeks(); const tx = await vault.connect(alice).withdraw(alice.address, [1]); - expect(await underlying.balanceOf(vault.address)).to.eq('1'); + // this should be zero and not one (to account dust) as per the above comment on line 324 + expect(await underlying.balanceOf(vault.address)).to.eq('0'); expect(await underlying.balanceOf(strategy.address)).to.eq( parseUnits('135'), ); @@ -363,7 +364,7 @@ describe('Vault in sync mode', () => { // vault had 115 before the withdrawal, and 100 was withdrawn, so it should have 115 - 100 = 15 expect(await underlying.balanceOf(vault.address)).to.eq( - '15000000000000000001'.toString(), + parseUnits('15').toString(), ); expect(await underlying.balanceOf(strategy.address)).to.eq( parseUnits('135'), @@ -389,7 +390,7 @@ describe('Vault in sync mode', () => { .partialWithdraw(alice.address, [1], [parseUnits('50')]); expect(await underlying.balanceOf(alice.address)).to.eq( - '949999999999999999999', + parseUnits('950'), ); }); @@ -411,7 +412,7 @@ describe('Vault in sync mode', () => { .partialWithdraw(alice.address, [1], [parseUnits('50')]); expect(await underlying.balanceOf(alice.address)).to.eq( - '949999999999999999999', + parseUnits('950'), ); }); @@ -433,7 +434,7 @@ describe('Vault in sync mode', () => { expect(await underlying.balanceOf(vault.address)).to.eq(parseUnits('10')); expect(await underlying.balanceOf(strategy.address)).to.eq( - '90000000000000000001', + parseUnits('90'), ); }); @@ -484,7 +485,7 @@ describe('Vault in sync mode', () => { .connect(alice) .partialWithdraw(alice.address, [1], [parseUnits('50')]); - expect(await underlying.balanceOf(vault.address)).to.eq('1'); + expect(await underlying.balanceOf(vault.address)).to.eq('0'); expect(await underlying.balanceOf(strategy.address)).to.eq( parseUnits('135'), ); @@ -516,9 +517,7 @@ describe('Vault in sync mode', () => { .partialWithdraw(alice.address, [1], [parseUnits('50')]); // vault had 115 before the withdrawal, and 50 was withdrawn, so it should have 115 - 50 = 65 - expect(await underlying.balanceOf(vault.address)).to.eq( - '65000000000000000001'.toString(), - ); + expect(await underlying.balanceOf(vault.address)).to.eq(parseUnits('65')); expect(await underlying.balanceOf(strategy.address)).to.eq( parseUnits('135'), );