From 79b7d9676a002bf4562a664e76081cc6e9718f5a Mon Sep 17 00:00:00 2001 From: chefburger Date: Wed, 3 Apr 2024 17:11:55 +0800 Subject: [PATCH] optimization: support burn onbehalf of other and uniform the signature of mint and burn function --- ...gerTest#swap_leaveSurplusTokenInVault.snap | 2 +- ...nagerTest#swap_useSurplusTokenAsInput.snap | 2 +- .forge-snapshots/VaultTest#Vault.snap | 2 +- .forge-snapshots/VaultTest#collectFee.snap | 2 +- .../VaultTest#registerPoolManager.snap | 2 +- .forge-snapshots/VaultTest#testLock_NoOp.snap | 2 +- src/Vault.sol | 6 +- src/VaultToken.sol | 9 +++ src/interfaces/IVault.sol | 4 +- test/pool-bin/helpers/BinSwapHelper.sol | 8 +-- test/pool-cl/helpers/CLPoolManagerRouter.sol | 8 +-- test/vault/FakePoolManagerRouter.sol | 20 ++++-- test/vault/Vault.t.sol | 68 +++++++++++++++++++ test/vault/VaultInvariant.t.sol | 8 +-- test/vault/VaultReentrancy.t.sol | 6 +- 15 files changed, 117 insertions(+), 32 deletions(-) diff --git a/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap b/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap index 107cd3d2..b2f018af 100644 --- a/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap +++ b/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap @@ -1 +1 @@ -102919 \ No newline at end of file +102897 \ No newline at end of file diff --git a/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap b/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap index 1f0b6296..da28d207 100644 --- a/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap +++ b/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap @@ -1 +1 @@ -101418 \ No newline at end of file +101659 \ No newline at end of file diff --git a/.forge-snapshots/VaultTest#Vault.snap b/.forge-snapshots/VaultTest#Vault.snap index 85d18717..a75c38b3 100644 --- a/.forge-snapshots/VaultTest#Vault.snap +++ b/.forge-snapshots/VaultTest#Vault.snap @@ -1 +1 @@ -7180 \ No newline at end of file +7314 \ No newline at end of file diff --git a/.forge-snapshots/VaultTest#collectFee.snap b/.forge-snapshots/VaultTest#collectFee.snap index 00565c3e..df5ec7a9 100644 --- a/.forge-snapshots/VaultTest#collectFee.snap +++ b/.forge-snapshots/VaultTest#collectFee.snap @@ -1 +1 @@ -25531 \ No newline at end of file +25576 \ No newline at end of file diff --git a/.forge-snapshots/VaultTest#registerPoolManager.snap b/.forge-snapshots/VaultTest#registerPoolManager.snap index 67481fbe..f8a88baa 100644 --- a/.forge-snapshots/VaultTest#registerPoolManager.snap +++ b/.forge-snapshots/VaultTest#registerPoolManager.snap @@ -1 +1 @@ -24506 \ No newline at end of file +24484 \ No newline at end of file diff --git a/.forge-snapshots/VaultTest#testLock_NoOp.snap b/.forge-snapshots/VaultTest#testLock_NoOp.snap index da1349a0..23cdb97c 100644 --- a/.forge-snapshots/VaultTest#testLock_NoOp.snap +++ b/.forge-snapshots/VaultTest#testLock_NoOp.snap @@ -1 +1 @@ -11232 \ No newline at end of file +11327 \ No newline at end of file diff --git a/src/Vault.sol b/src/Vault.sol index 64d8c6a2..749104db 100644 --- a/src/Vault.sol +++ b/src/Vault.sol @@ -112,7 +112,7 @@ contract Vault is IVault, VaultToken, Ownable { } /// @inheritdoc IVault - function mint(Currency currency, address to, uint256 amount) external override isLocked { + function mint(address to, Currency currency, uint256 amount) external override isLocked { SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128()); _mint(to, currency, amount); } @@ -161,9 +161,9 @@ contract Vault is IVault, VaultToken, Ownable { } /// @inheritdoc IVault - function burn(Currency currency, uint256 amount) external override isLocked { + function burn(address from, Currency currency, uint256 amount) external override isLocked { SettlementGuard.accountDelta(msg.sender, currency, -(amount.toInt128())); - _burn(msg.sender, currency, amount); + _burnFrom(from, currency, amount); } /// @inheritdoc IVault diff --git a/src/VaultToken.sol b/src/VaultToken.sol index 8251622b..d27468ed 100644 --- a/src/VaultToken.sol +++ b/src/VaultToken.sol @@ -98,4 +98,13 @@ abstract contract VaultToken is IVaultToken { emit Transfer(msg.sender, sender, address(0), currency, amount); } + + function _burnFrom(address from, Currency currency, uint256 amount) internal virtual { + if (msg.sender != from && !isOperator[from][msg.sender]) { + uint256 allowed = allowance[from][msg.sender][currency]; + if (allowed != type(uint256).max) allowance[from][msg.sender][currency] = allowed - amount; + } + + _burn(from, currency, amount); + } } diff --git a/src/interfaces/IVault.sol b/src/interfaces/IVault.sol index ea8d905b..b9ac6194 100644 --- a/src/interfaces/IVault.sol +++ b/src/interfaces/IVault.sol @@ -89,8 +89,8 @@ interface IVault is IVaultToken { function collectFee(Currency currency, uint256 amount, address recipient) external; /// @notice Called by the user to store surplus tokens in the vault - function mint(Currency currency, address to, uint256 amount) external; + function mint(address to, Currency currency, uint256 amount) external; /// @notice Called by the user to use surplus tokens for payment settlement - function burn(Currency currency, uint256 amount) external; + function burn(address from, Currency currency, uint256 amount) external; } diff --git a/test/pool-bin/helpers/BinSwapHelper.sol b/test/pool-bin/helpers/BinSwapHelper.sol index 66312fb8..fc75b53b 100644 --- a/test/pool-bin/helpers/BinSwapHelper.sol +++ b/test/pool-bin/helpers/BinSwapHelper.sol @@ -83,14 +83,14 @@ contract BinSwapHelper { } else { // the received hook on this transfer will burn the tokens vault.transferFrom(data.sender, address(this), data.key.currency0, uint128(delta.amount0())); - vault.burn(data.key.currency0, uint128(delta.amount0())); + vault.burn(address(this), data.key.currency0, uint128(delta.amount0())); } } if (delta.amount1() < 0) { if (data.testSettings.withdrawTokens) { vault.take(data.key.currency1, data.sender, uint128(-delta.amount1())); } else { - vault.mint(data.key.currency1, data.sender, uint128(-delta.amount1())); + vault.mint(data.sender, data.key.currency1, uint128(-delta.amount1())); } } } else { @@ -107,14 +107,14 @@ contract BinSwapHelper { } else { // the received hook on this transfer will burn the tokens vault.transferFrom(data.sender, address(this), data.key.currency1, uint128(delta.amount1())); - vault.burn(data.key.currency1, uint128(delta.amount1())); + vault.burn(address(this), data.key.currency1, uint128(delta.amount1())); } } if (delta.amount0() < 0) { if (data.testSettings.withdrawTokens) { vault.take(data.key.currency0, data.sender, uint128(-delta.amount0())); } else { - vault.mint(data.key.currency0, data.sender, uint128(-delta.amount0())); + vault.mint(data.sender, data.key.currency0, uint128(-delta.amount0())); } } } diff --git a/test/pool-cl/helpers/CLPoolManagerRouter.sol b/test/pool-cl/helpers/CLPoolManagerRouter.sol index c73609a5..08e423dc 100644 --- a/test/pool-cl/helpers/CLPoolManagerRouter.sol +++ b/test/pool-cl/helpers/CLPoolManagerRouter.sol @@ -159,14 +159,14 @@ contract CLPoolManagerRouter { } else { // the received hook on this transfer will burn the tokens vault.transferFrom(data.sender, address(this), data.key.currency0, uint128(delta.amount0())); - vault.burn(data.key.currency0, uint128(delta.amount0())); + vault.burn(address(this), data.key.currency0, uint128(delta.amount0())); } } if (delta.amount1() < 0) { if (data.testSettings.withdrawTokens) { vault.take(data.key.currency1, data.sender, uint128(-delta.amount1())); } else { - vault.mint(data.key.currency1, data.sender, uint128(-delta.amount1())); + vault.mint(data.sender, data.key.currency1, uint128(-delta.amount1())); } } } else { @@ -183,14 +183,14 @@ contract CLPoolManagerRouter { } else { // the received hook on this transfer will burn the tokens vault.transferFrom(data.sender, address(this), data.key.currency1, uint128(delta.amount1())); - vault.burn(data.key.currency1, uint128(delta.amount1())); + vault.burn(address(this), data.key.currency1, uint128(delta.amount1())); } } if (delta.amount0() < 0) { if (data.testSettings.withdrawTokens) { vault.take(data.key.currency0, data.sender, uint128(-delta.amount0())); } else { - vault.mint(data.key.currency0, data.sender, uint128(-delta.amount0())); + vault.mint(data.sender, data.key.currency0, uint128(-delta.amount0())); } } } diff --git a/test/vault/FakePoolManagerRouter.sol b/test/vault/FakePoolManagerRouter.sol index ccda2b48..36d3fbda 100644 --- a/test/vault/FakePoolManagerRouter.sol +++ b/test/vault/FakePoolManagerRouter.sol @@ -99,20 +99,20 @@ contract FakePoolManagerRouter { // mint uint256 amt = poolKey.currency0.balanceOf(address(vault)); vault.settle(poolKey.currency0); - vault.mint(poolKey.currency0, address(this), amt); + vault.mint(address(this), poolKey.currency0, amt); } else if (data[0] == 0x14) { // mint to someone else, poolKey.currency1 for example uint256 amt = poolKey.currency0.balanceOf(address(vault)); vault.settle(poolKey.currency0); - vault.mint(poolKey.currency0, Currency.unwrap(poolKey.currency1), amt); + vault.mint(Currency.unwrap(poolKey.currency1), poolKey.currency0, amt); } else if (data[0] == 0x15) { // burn uint256 amt = poolKey.currency0.balanceOf(address(vault)); vault.settle(poolKey.currency0); - vault.mint(poolKey.currency0, address(this), amt); + vault.mint(address(this), poolKey.currency0, amt); - vault.burn(poolKey.currency0, amt); + vault.burn(address(this), poolKey.currency0, amt); vault.take(poolKey.currency0, address(this), amt); } else if (data[0] == 0x16) { // burn half if possible @@ -120,9 +120,9 @@ contract FakePoolManagerRouter { uint256 amt = poolKey.currency0.balanceOf(address(vault)); vault.settle(poolKey.currency0); - vault.mint(poolKey.currency0, address(this), amt); + vault.mint(address(this), poolKey.currency0, amt); - vault.burn(poolKey.currency0, amt / 2); + vault.burn(address(this), poolKey.currency0, amt / 2); vault.take(poolKey.currency0, address(this), amt / 2); } else if (data[0] == 0x17) { // settle ETH @@ -140,6 +140,14 @@ contract FakePoolManagerRouter { /// try to call settleAndMintRefund should not revert vault.settleAndMintRefund(poolKey.currency1, address(this)); vault.take(poolKey.currency1, address(this), 3 ether); + } else if (data[0] == 0x20) { + // burn on behalf of someone else + uint256 amt = poolKey.currency0.balanceOf(address(vault)); + vault.settle(poolKey.currency0); + vault.mint(address(0x01), poolKey.currency0, amt); + + vault.burn(address(0x01), poolKey.currency0, amt); + vault.take(poolKey.currency0, address(this), amt); } return ""; diff --git a/test/vault/Vault.t.sol b/test/vault/Vault.t.sol index df103441..dd49bc57 100644 --- a/test/vault/Vault.t.sol +++ b/test/vault/Vault.t.sol @@ -413,6 +413,74 @@ contract VaultTest is Test, GasSnapshot { assertEq(vault.balanceOf(address(fakePoolManagerRouter), currency0), amt - amt / 2); } + function testVaultFuzz_burnFrom_withoutApprove(uint256 amt) public { + amt = bound(amt, 0, 10 ether); + // make sure router has enough tokens + currency0.transfer(address(vault), amt); + + if (amt != 0) { + vm.expectRevert(); + } + + vm.startPrank(address(fakePoolManagerRouter)); + vault.lock(hex"20"); + vm.stopPrank(); + + assertEq(vault.balanceOf(address(fakePoolManagerRouter), currency0), 0); + } + + function testVaultFuzz_burnFrom_withApprove(uint256 amt) public { + amt = bound(amt, 0, 10 ether); + // make sure router has enough tokens + currency0.transfer(address(vault), amt); + + vm.prank(address(0x01)); + vault.approve(address(fakePoolManagerRouter), currency0, amt); + assertEq(vault.allowance(address(0x01), address(fakePoolManagerRouter), currency0), amt); + + vm.startPrank(address(fakePoolManagerRouter)); + vault.lock(hex"20"); + vm.stopPrank(); + + assertEq(vault.balanceOf(address(fakePoolManagerRouter), currency0), 0); + assertEq(vault.allowance(address(0x01), address(fakePoolManagerRouter), currency0), 0); + + // approve max + { + currency0.transfer(address(vault), amt); + + vm.prank(address(0x01)); + vault.approve(address(fakePoolManagerRouter), currency0, type(uint256).max); + + vm.startPrank(address(fakePoolManagerRouter)); + vault.lock(hex"20"); + vm.stopPrank(); + + assertEq(vault.balanceOf(address(fakePoolManagerRouter), currency0), 0); + assertEq(vault.allowance(address(0x01), address(fakePoolManagerRouter), currency0), type(uint256).max); + } + + // operator + { + currency0.transfer(address(vault), amt); + + // set a insufficient allowance + vm.prank(address(0x01)); + vault.approve(address(fakePoolManagerRouter), currency0, 1); + + vm.prank(address(0x01)); + vault.setOperator(address(fakePoolManagerRouter), true); + + vm.startPrank(address(fakePoolManagerRouter)); + vault.lock(hex"20"); + vm.stopPrank(); + + assertEq(vault.balanceOf(address(fakePoolManagerRouter), currency0), 0); + // transfer from operator don't consume allowance + assertEq(vault.allowance(address(0x01), address(fakePoolManagerRouter), currency0), 1); + } + } + function testLockInSufficientBalanceWhenMoreThanOnePoolManagers() public { // router => vault.lock // vault.lock => periphery.lockAcquired diff --git a/test/vault/VaultInvariant.t.sol b/test/vault/VaultInvariant.t.sol index fe05de01..398fba2c 100644 --- a/test/vault/VaultInvariant.t.sol +++ b/test/vault/VaultInvariant.t.sol @@ -168,8 +168,8 @@ contract VaultPoolManager is Test { BalanceDelta delta = toBalanceDelta(-(int128(action.amt0)), -(int128(action.amt1))); vault.accountPoolBalanceDelta(poolKey, delta, address(this)); - vault.mint(currency0, address(this), action.amt0); - vault.mint(currency1, address(this), action.amt1); + vault.mint(address(this), currency0, action.amt0); + vault.mint(address(this), currency1, action.amt1); totalMintedCurrency0 += action.amt0; totalMintedCurrency1 += action.amt1; } else if (action.actionType == ActionType.Settle) { @@ -212,8 +212,8 @@ contract VaultPoolManager is Test { BalanceDelta delta = toBalanceDelta(int128(action.amt0), int128(action.amt1)); vault.accountPoolBalanceDelta(poolKey, delta, address(this)); - vault.burn(currency0, action.amt0); - vault.burn(currency1, action.amt1); + vault.burn(address(this), currency0, action.amt0); + vault.burn(address(this), currency1, action.amt1); totalMintedCurrency0 -= action.amt0; totalMintedCurrency1 -= action.amt1; } diff --git a/test/vault/VaultReentrancy.t.sol b/test/vault/VaultReentrancy.t.sol index 18a1b2b9..6ced7bd9 100644 --- a/test/vault/VaultReentrancy.t.sol +++ b/test/vault/VaultReentrancy.t.sol @@ -158,7 +158,7 @@ contract VaultReentrancyTest is Test, TokenFixture { vm.prank(callerAddr); vault.settle(currency0); vm.prank(callerAddr); - vault.mint(currency0, address(callerAddr), 1 ether); + vault.mint(address(callerAddr), currency0, 1 ether); vaultTokenBalance[i] = vault.balanceOf(callerAddr, currency0); } @@ -194,14 +194,14 @@ contract VaultReentrancyTest is Test, TokenFixture { } else if (i % 6 == 2) { // mint vm.prank(callerAddr); - vault.mint(currency0, callerAddr, paidAmount); + vault.mint(callerAddr, currency0, paidAmount); currencyDelta[i % SETTLERS_AMOUNT] += int256(paidAmount); vaultTokenBalance[i % SETTLERS_AMOUNT] += paidAmount; } else if (i % 6 == 3) { // burn vm.prank(callerAddr); - vault.burn(currency0, paidAmount); + vault.burn(callerAddr, currency0, paidAmount); currencyDelta[i % SETTLERS_AMOUNT] -= int256(paidAmount); vaultTokenBalance[i % SETTLERS_AMOUNT] -= paidAmount;