feat: fix settleAndRefund vulnerability (#4)

* feat: update from currency.transfer to mint

* refactor: update logic to be more similar to settle

* gas: minor gas tweak - reduce repetitive casting

.forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap

@@ -1 +1 @@
-92310
+92245

.forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap

@@ -1 +1 @@
-67502
+67437

.forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap

@@ -1 +1 @@
-151057
+150992

.forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap

@@ -1 +1 @@
-68681
+68616

.forge-snapshots/BinPoolManagerTest#testGasDonate.snap

@@ -1 +1 @@
-53973
+53952

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap

@@ -1 +1 @@
-968068
+968047

.forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap

@@ -1 +1 @@
-121582
+121561

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap

@@ -1 +1 @@
-337258
+337237

.forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap

@@ -1 +1 @@
-56319
+56298

.forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap

@@ -1 +1 @@
-93083
+93040

.forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap

@@ -1 +1 @@
-95068
+95025

.forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap

@@ -1 +1 @@
-74512
+74469

.forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap

@@ -1 +1 @@
-318966
+318945

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Burn.snap

@@ -1 +1 @@
-41941
+41876

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Donate.snap

@@ -1 +1 @@
-19558
+19493

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Mint.snap

@@ -1 +1 @@
-37410
+37345

.forge-snapshots/BinPoolManagerTest#testNoOpGas_Swap.snap

@@ -1 +1 @@
-22764
+22699

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap

@@ -1 +1 @@
-348473
+348452

.forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap

@@ -1 +1 @@
-61021
+61000

.forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap

@@ -1 +1 @@
-241433
+241390

.forge-snapshots/CLPoolManagerTest#donateBothTokens.snap

@@ -1 +1 @@
-84159
+84138

.forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap

@@ -1 +1 @@
-53488
+53445

.forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap

@@ -1 +1 @@
-43387
+43322

.forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap

@@ -1 +1 @@
-56488
+56445

.forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap

@@ -1 +1 @@
-104597
+104543

.forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap

@@ -1 +1 @@
-25044312
+25044269

.forge-snapshots/CLPoolManagerTest#swap_simple.snap

@@ -1 +1 @@
-36401
+36336

.forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap

@@ -1 +1 @@
-103140
+103041

.forge-snapshots/CLPoolManagerTest#swap_withHooks.snap

@@ -1 +1 @@
-42051
+41986

.forge-snapshots/CLPoolManagerTest#swap_withNative.snap

@@ -1 +1 @@
-36404
+36339

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_Donate.snap

@@ -1 +1 @@
-19513
+19448

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_ModifyPosition.snap

@@ -1 +1 @@
-27680
+27615

.forge-snapshots/CLPoolManagerTest#testNoOp_gas_Swap.snap

@@ -1 +1 @@
-21962
+21897

.forge-snapshots/VaultTest#Vault.snap

@@ -1 +1 @@
-7075
+7033

.forge-snapshots/VaultTest#lockSettledWhenAddLiquidity.snap

@@ -1 +1 @@
-122540
+122519

.forge-snapshots/VaultTest#lockSettledWhenFlashloan.snap

@@ -1 +1 @@
-158832
+158811

.forge-snapshots/VaultTest#lockSettledWhenMultiHopSwap.snap

@@ -1 +1 @@
-47017
+46974

.forge-snapshots/VaultTest#lockSettledWhenSwap.snap

@@ -1 +1 @@
-47016
+46973

.forge-snapshots/VaultTest#testLock_NoOp.snap

@@ -1 +1 @@
-11692
+11627

.forge-snapshots/VaultTest#testSettleAndMintRefund_WithMint.snap

@@ -0,0 +1 @@
+72506

.forge-snapshots/VaultTest#testSettleAndMintRefund_WithoutMint.snap

@@ -0,0 +1 @@
+34113

src/Vault.sol

@@ -126,26 +126,29 @@ contract Vault is IVault, VaultToken, Ownable {
         SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
     }
 
-    function settleAndRefund(Currency currency, address to)
+    function settleAndMintRefund(Currency currency, address to)
         external
         payable
         override
         isLocked
         returns (uint256 paid, uint256 refund)
     {
-        paid = currency.balanceOfSelf() - reservesOfVault[currency];
-        int256 currentDelta = SettlementGuard.getCurrencyDelta(msg.sender, currency);
+        uint256 reservesBefore = reservesOfVault[currency];
+        reservesOfVault[currency] = currency.balanceOfSelf();
+        paid = reservesOfVault[currency] - reservesBefore;
 
-        if (currentDelta >= 0 && paid > currentDelta.toUint256()) {
-            // msg.sender owes vault but paid more than than whats owed
-            refund = paid - currentDelta.toUint256();
-            paid = currentDelta.toUint256();
+        int256 currentDelta = SettlementGuard.getCurrencyDelta(msg.sender, currency);
+        if (currentDelta >= 0) {
+            uint256 currentDeltaUint256 = currentDelta.toUint256();
+            if (paid > currentDeltaUint256) {
+                // msg.sender owes vault but paid more than than whats owed
+                refund = paid - currentDeltaUint256;
+                paid = currentDeltaUint256;
+            }
         }
 
-        reservesOfVault[currency] += paid;
         SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
-
-        if (refund > 0) currency.transfer(to, refund);
+        if (refund > 0) _mint(to, currency, refund);
     }
 
     /// @inheritdoc IVault

src/interfaces/IVault.sol

@@ -67,12 +67,16 @@ interface IVault is IVaultToken {
     /// @notice Called by the user to pay what is owed
     function settle(Currency token) external payable returns (uint256 paid);
 
-    /// @notice Called by the user to pay what is owed. If the payment is more than the debt, the surplus is refunded
+    /// @notice Called by the user to pay what is owed. If the payment is more than the debt, the surplus is refunded by minting
+    /// @dev To claim the refund, caller must eventually call vault.burn() -> vault.take() to take the ERC20 token from vault
     /// @param currency The currency to settle
-    /// @param to The address to refund the surplus to
+    /// @param to The address to mint the refund
     /// @return paid The amount paid
     /// @return refund The amount refunded
-    function settleAndRefund(Currency currency, address to) external payable returns (uint256 paid, uint256 refund);
+    function settleAndMintRefund(Currency currency, address to)
+        external
+        payable
+        returns (uint256 paid, uint256 refund);
 
     /// @notice move the delta from target to the msg.sender, only payment delta can be moved
     /// @param currency The currency to settle

test/vault/FakePoolManagerRouter.sol

@@ -131,14 +131,14 @@ contract FakePoolManagerRouter {
         } else if (data[0] == 0x18) {
             // call this method via vault.lock(abi.encodePacked(hex"18", alice));
             address to = address(uint160(uint256(bytes32(data[1:0x15]) >> 96)));
-            vault.settleAndRefund(poolKey.currency0, to);
-            vault.settleAndRefund(poolKey.currency1, to);
+            vault.settleAndMintRefund(poolKey.currency0, to);
+            vault.settleAndMintRefund(poolKey.currency1, to);
         } else if (data[0] == 0x19) {
             poolManager.mockAccounting(poolKey, 3 ether, -3 ether);
             vault.settle(poolKey.currency0);
 
-            /// try to call settleAndRefund should not revert
-            vault.settleAndRefund(poolKey.currency1, address(this));
+            /// try to call settleAndMintRefund should not revert
+            vault.settleAndMintRefund(poolKey.currency1, address(this));
             vault.take(poolKey.currency1, address(this), 3 ether);
         }
 

test/vault/Vault.t.sol

@@ -173,43 +173,39 @@ contract VaultTest is Test, GasSnapshot {
         vault.lock(hex"02");
     }
 
-    function testSettleAndRefund_WithErc20Transfer() public {
+    function testSettleAndMintRefund_WithMint() public {
         address alice = makeAddr("alice");
 
         // simulate someone transferred token to vault
         currency0.transfer(address(vault), 10 ether);
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);
+        assertEq(vault.balanceOf(alice, currency0), 0 ether);
 
         // settle and refund
         vm.prank(address(fakePoolManagerRouter));
-        snapStart("VaultTest#testSettleAndRefund_WithErc20Transfer");
+        snapStart("VaultTest#testSettleAndMintRefund_WithMint");
         vault.lock(abi.encodePacked(hex"18", alice));
         snapEnd();
 
-        // verify
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 10 ether);
+        // verify excess currency minted to alice
+        assertEq(vault.balanceOf(alice, currency0), 10 ether);
     }
 
-    function testSettleAndRefund_WithoutErc20Transfer() public {
+    function testSettleAndMintRefund_WithoutMint() public {
         address alice = makeAddr("alice");
 
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);
+        assertEq(vault.balanceOf(alice, currency0), 0 ether);
 
         // settleAndRefund works even if there's no excess currency
         vm.prank(address(fakePoolManagerRouter));
-        snapStart("VaultTest#testSettleAndRefund_WithoutErc20Transfer");
+        snapStart("VaultTest#testSettleAndMintRefund_WithoutMint");
         vault.lock(abi.encodePacked(hex"18", alice));
         snapEnd();
 
-        // verify
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(fakePoolManagerRouter)), 0 ether);
-        assertEq(IERC20(Currency.unwrap(currency0)).balanceOf(address(alice)), 0 ether);
+        // verify no extra token minted
+        assertEq(vault.balanceOf(alice, currency0), 0 ether);
     }
 
-    function testSettleAndRefund_NegativeBalanceDelta() public {
+    function testSettleAndMintRefund_NegativeBalanceDelta() public {
         // pre-req: ensure vault has some value in reserveOfVault[] before
         currency0.transfer(address(vault), 10 ether);
         currency1.transfer(address(vault), 10 ether);

test/vault/VaultInvariant.t.sol

@@ -31,7 +31,7 @@ contract VaultPoolManager is Test {
     enum ActionType {
         Take,
         Settle,
-        SettleAndRefund,
+        SettleAndMintRefund,
         SettleFor,
         Mint,
         Burn
@@ -87,7 +87,7 @@ contract VaultPoolManager is Test {
 
     /// @dev In settleAndRefund case, assume user add liquidity and paying to the vault
     ///      but theres another folk who minted extra token to the vault
-    function settleAndRefund(uint256 amt0, uint256 amt1, bool sendToVault) public {
+    function settleAndMintRefund(uint256 amt0, uint256 amt1, bool sendToVault) public {
         amt0 = bound(amt0, 0, MAX_TOKEN_BALANCE - 1 ether);
         amt1 = bound(amt1, 0, MAX_TOKEN_BALANCE - 1 ether);
 
@@ -97,7 +97,7 @@ contract VaultPoolManager is Test {
         // mint token to VaultPoolManager, so VaultPoolManager can pay to the vault
         token0.mint(address(this), amt0);
         token1.mint(address(this), amt1);
-        vault.lock(abi.encode(Action(ActionType.SettleAndRefund, uint128(amt0), uint128(amt1))));
+        vault.lock(abi.encode(Action(ActionType.SettleAndMintRefund, uint128(amt0), uint128(amt1))));
     }
 
     /// @dev In settleFor case, assume user is paying for hook
@@ -181,15 +181,18 @@ contract VaultPoolManager is Test {
 
             vault.settle(currency0);
             vault.settle(currency1);
-        } else if (action.actionType == ActionType.SettleAndRefund) {
+        } else if (action.actionType == ActionType.SettleAndMintRefund) {
             BalanceDelta delta = toBalanceDelta(int128(action.amt0), int128(action.amt1));
             vault.accountPoolBalanceDelta(poolKey, delta, address(this));
 
             token0.transfer(address(vault), action.amt0);
             token1.transfer(address(vault), action.amt1);
 
-            vault.settleAndRefund(currency0, address(this));
-            vault.settleAndRefund(currency1, address(this));
+            (, uint256 refund0) = vault.settleAndMintRefund(currency0, address(this));
+            (, uint256 refund1) = vault.settleAndMintRefund(currency1, address(this));
+
+            totalMintedCurrency0 += refund0;
+            totalMintedCurrency1 += refund1;
         } else if (action.actionType == ActionType.SettleFor) {
             // hook cash out the fee ahead
             BalanceDelta delta = toBalanceDelta(int128(action.amt0), int128(action.amt1));
@@ -244,7 +247,7 @@ contract VaultInvariant is Test, GasSnapshot {
         selectors[3] = VaultPoolManager.burn.selector;
         selectors[4] = VaultPoolManager.settleFor.selector;
         selectors[5] = VaultPoolManager.collectFee.selector;
-        selectors[6] = VaultPoolManager.settleAndRefund.selector;
+        selectors[6] = VaultPoolManager.settleAndMintRefund.selector;
         targetSelector(FuzzSelector({addr: address(vaultPoolManager), selectors: selectors}));
     }