optimization: support burn onbehalf of other and uniform the signatur…

…e of mint and burn function (#14)
pancakeswap · Apr 12, 2024 · a3cf6ea · a3cf6ea
1 parent b41cd40
commit a3cf6ea
Show file tree

Hide file tree

Showing 15 changed files with 117 additions and 32 deletions.
diff --git a/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap b/.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap
@@ -1 +1 @@
-102920
+102898
diff --git a/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap b/.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap
@@ -1 +1 @@
-101419
+101660
diff --git a/.forge-snapshots/VaultTest#Vault.snap b/.forge-snapshots/VaultTest#Vault.snap
@@ -1 +1 @@
-7180
+7314
diff --git a/.forge-snapshots/VaultTest#collectFee.snap b/.forge-snapshots/VaultTest#collectFee.snap
@@ -1 +1 @@
-25531
+25576
diff --git a/.forge-snapshots/VaultTest#registerPoolManager.snap b/.forge-snapshots/VaultTest#registerPoolManager.snap
@@ -1 +1 @@
-24506
+24484
diff --git a/.forge-snapshots/VaultTest#testLock_NoOp.snap b/.forge-snapshots/VaultTest#testLock_NoOp.snap
@@ -1 +1 @@
-11232
+11327
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -99,30 +99,30 @@ 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
 
             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
@@ -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
@@ -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
@@ -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;