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.

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

Add settleAndRefund #2

Merged
merged 5 commits into from
Mar 21, 2024
Merged

Add settleAndRefund #2

merged 5 commits into from
Mar 21, 2024

Conversation

ChefMist
Copy link
Collaborator

@ChefMist ChefMist commented Mar 20, 2024

Description

In normal cases, user would transfer the token to the vault and call vault.settle() eg.

// add liquidity 
BalanceDelta delta = poolManager.modifyLiquidity

// transfer token to vault and settle 
token0.transfer(address(vault), delta.amount0);
vault.settle(currency0);

token1.transfer(address(vault), delta.amount1);
vault.settle(currency1);

However, someone can potentially cause the txn to fail with CurrencyNotSettled() by sending 1 wei of token0 or token1 into the vault.

Fix

This PR adds settleAndRefund which handle this cases. It will be used by the v4-periphery contracts and should help to remove excess tokens in vault.

src/Vault.sol Outdated Show resolved Hide resolved
@chefburger
Copy link
Collaborator

rest lgtm

src/Vault.sol Outdated
isLocked
returns (uint256 paid, uint256 refund)
{
paid = settle(currency);
Copy link
Collaborator

@ChefSnoopy ChefSnoopy Mar 20, 2024

Choose a reason for hiding this comment

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

Should we calculate the amount we need to refund in advance? then just need to execute SettlementGuard.accountDelta once if have excess token.

function settleAndRefund(Currency currency, address to)
        external
        payable
        override
        isLocked
        returns (uint256 paid, uint256 refund)
    {
        paid = currency.balanceOfSelf() - reservesOfVault[currency];
        // need to consider the case when the currencyDelta is negative ?.
        uint256 currentCurrencyDelta = (SettlementGuard.getCurrencyDelta(msg.sender, currency)).toUint256();
       
        if (paid > currentCurrencyDelta) {
            refund = paid - currentCurrencyDelta;
            paid = currentCurrencyDelta;
        }
        reservesOfVault[currency] += paid;
        SettlementGuard.accountDelta(msg.sender, currency, -(paid.toInt128()));
        if (refund > 0) {
            currency.transfer(to, refund);
        }
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! adopted this idea since it'll save 1 sload for refund case. though did a small tweak to handle negative currency delta as settle() wont revert with SafeCastOverflow for negative balance delta

@@ -0,0 +1 @@
56136
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was 79085 gas previously and now dip to 56136 with Snoopy's implementation

src/Vault.sol Outdated
// msg.sender owes vault but paid more than than whats owed
refund = paid - currentDelta.toUint256();
paid = currentDelta.toUint256();
currency.transfer(to, refund);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to think about whether there are re-entrant issues.
Let me think about this. maybe need to put this in the end after updated reservesOfVault and SettlementGuard.

@ChefMist ChefMist merged commit ffd20cb into main Mar 21, 2024
2 checks passed
@ChefMist ChefMist deleted the feat/settle-and-refund branch March 21, 2024 06:57
cyuxhtby added a commit to cyuxhtby/pancake-v4-core that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants