Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fix settleAndRefund vulnerability #4

Merged
merged 3 commits into from
Mar 25, 2024
Merged

feat: fix settleAndRefund vulnerability #4

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a4cb02
feat: update from currency.transfer to mint
ChefMist Mar 22, 2024
7b07b9d
refactor: update logic to be more similar to settle
ChefMist Mar 22, 2024
9461d4c
gas: minor gas tweak - reduce repetitive casting
ChefMist Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testBurnNativeCurrency.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
92310
92245
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnHalfBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
67502
67437
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnNineBins.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
151057
150992
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasBurnOneBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
68681
68616
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasDonate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
53973
53952
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintNneBins-1.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
968068
968047
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintNneBins-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
121582
121561
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintOneBin-1.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
337258
337237
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasMintOneBin-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56319
56298
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapMultipleBins.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
93083
93040
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapOverBigBinIdGate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
95068
95025
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testGasSwapSingleBin.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
74512
74469
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testMintNativeCurrency.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
318966
318945
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Burn.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
41941
41876
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Donate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19558
19493
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Mint.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
37410
37345
2 changes: 1 addition & 1 deletion .forge-snapshots/BinPoolManagerTest#testNoOpGas_Swap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
22764
22699
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_fromEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
348473
348452
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_fromNonEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
61021
61000
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#addLiquidity_nativeToken.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
241433
241390
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#donateBothTokens.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
84159
84138
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#gasDonateOneToken.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
53488
53445
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#removeLiquidity_toNonEmpty.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
43387
43322
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_againstLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56488
56445
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_leaveSurplusTokenInVault.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
104597
104543
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_runOutOfLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25044312
25044269
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_simple.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
36401
36336
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_useSurplusTokenAsInput.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
103140
103041
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withHooks.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
42051
41986
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#swap_withNative.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
36404
36339
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#testNoOp_gas_Donate.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
19513
19448
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#testNoOp_gas_ModifyPosition.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
27680
27615
2 changes: 1 addition & 1 deletion .forge-snapshots/CLPoolManagerTest#testNoOp_gas_Swap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
21962
21897
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#Vault.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
7075
7049
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenAddLiquidity.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
122540
122519
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenFlashloan.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
158832
158811
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenMultiHopSwap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
47017
46974
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#lockSettledWhenSwap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
47016
46973
2 changes: 1 addition & 1 deletion .forge-snapshots/VaultTest#testLock_NoOp.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
11692
11627
1 change: 1 addition & 0 deletions .forge-snapshots/VaultTest#testSettleAndMintRefund_WithMint.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
72621
1 change: 1 addition & 0 deletions .forge-snapshots/VaultTest#testSettleAndMintRefund_WithoutMint.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
34111
1 change: 0 additions & 1 deletion .forge-snapshots/VaultTest#testSettleAndRefund_WithErc20Transfer.snap
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion .forge-snapshots/VaultTest#testSettleAndRefund_WithoutErc20Transfer.snap
View file
Open in desktop

This file was deleted.

12 changes: 6 additions & 6 deletions src/Vault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,26 +126,26 @@ 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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this part, the function would revert if reservesBefore is larger than the current reserve, since it would mean that the locker did not transfer anything to increase the current balance right?

Copy link
Collaborator Author

@ChefMist ChefMist Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the locker didn't transfer anything, this function shouldn't revert. The behavior should be the same as settle().

Can think of 1 scenario for revert:
1/ might be for rebasing token, where a negative rebase was done which cause the current balance in vault to be lesser than reserveBefore

cc @chefburger or @ChefSnoopy if can add on to more scenarios where revert might happen

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cant come with other cases either, but i think it's fine to just let it revert if reservesBefore is greater than current.


int256 currentDelta = SettlementGuard.getCurrencyDelta(msg.sender, currency);
if (currentDelta >= 0 && paid > currentDelta.toUint256()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can cache currentDelta after casting to uint256 so you don't have to keep casting it again, wasting gas.

        if (currentDelta >= 0) {
            uint256 currentDeltaUint256 = currentDelta.toUint256();
            if (paid > currentDeltaUint256 ) {
                // msg.sender owes vault but paid more than whats owed
                refund = paid - currentDeltaUint256;
                paid = currentDeltaUint256;
            }
        }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just to confirm, refund is only done, and paid is only adjusted if currentDelta is positive or zero, never if currentDelta is negative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just to confirm, refund is only done, and paid is only adjusted if currentDelta is positive or zero, never if currentDelta is negative?

yep. do you think theres a scenario where negative balanceDelta (vault owes caller token) might need to handled?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, that's why I'm asking.

// msg.sender owes vault but paid more than than whats owed
refund = paid - currentDelta.toUint256();
paid = currentDelta.toUint256();
}

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
Expand Down
10 changes: 7 additions & 3 deletions src/interfaces/IVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions test/vault/FakePoolManagerRouter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
26 changes: 11 additions & 15 deletions test/vault/Vault.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 10 additions & 7 deletions test/vault/VaultInvariant.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ contract VaultPoolManager is Test {
enum ActionType {
Take,
Settle,
SettleAndRefund,
SettleAndMintRefund,
SettleFor,
Mint,
Burn
Expand Down Expand Up @@ -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);

Expand All @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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}));
}

Expand Down
Loading