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

Swap limit bug fix #221

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Swap limit bug fix #221

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
547840f
skipping liquidity check when swapping from perps to underlying
aalavandhan Aug 29, 2024
2e2204c
code review fixes
aalavandhan Aug 30, 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
32 changes: 13 additions & 19 deletions spot-contracts/contracts/RolloverVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,12 @@ contract RolloverVault is
// The vault continues to hold the perp dust until the subsequent `swapPerpsForUnderlying` or manual `recover(perp)`.

// Revert if vault liquidity is too low.
_enforceUnderlyingBalAfterSwap(underlying_, s.vaultTVL);
// - Absolute balance is strictly greater than `minUnderlyingBal`.
// - Ratio of the balance to the vault's TVL is strictly greater than `minUnderlyingPerc`.
uint256 underlyingBal = underlying_.balanceOf(address(this));
Copy link
Member

Choose a reason for hiding this comment

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

I guess there's a weird corner case where underlying balance could go up (or maybe just remain constant?) after an underlying->perp swap.
e.g. If the vault was sitting on a bunch of As, and the new Zs free those up.
In that case I think we'd want to allow that to go through.

Perhaps the most generalized logic would be:

If underlying bal increases or stays the same, continue
else, execute the check logic you have here.

That situation would be exceptionally rare, though. Worth it? It would be one extra var and a conditional, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool. So basically if the underlying balances goes down, we then ensure that it doesn't go down too much. If it stays the same or increases its fine..

if (underlyingBal <= minUnderlyingBal || underlyingBal.mulDiv(ONE, s.vaultTVL) <= minUnderlyingPerc) {
revert InsufficientLiquidity();
}

// sync underlying
_syncAsset(underlying_);
Expand All @@ -482,11 +487,8 @@ contract RolloverVault is
// Calculates the fee adjusted underlying amount to transfer to the user.
IPerpetualTranche perp_ = perp;
IERC20Upgradeable underlying_ = underlying;
(
uint256 underlyingAmtOut,
uint256 perpFeeAmtToBurn,
SubscriptionParams memory s
) = computePerpToUnderlyingSwapAmt(perpAmtIn);
uint256 underlyingBalPre = underlying_.balanceOf(address(this));
(uint256 underlyingAmtOut, uint256 perpFeeAmtToBurn, ) = computePerpToUnderlyingSwapAmt(perpAmtIn);

// Revert if insufficient tokens are swapped in or out
if (underlyingAmtOut <= 0 || perpAmtIn <= 0) {
Expand All @@ -507,8 +509,11 @@ contract RolloverVault is
// transfer underlying out
underlying_.safeTransfer(msg.sender, underlyingAmtOut);

// Revert if vault liquidity is too low.
_enforceUnderlyingBalAfterSwap(underlying_, s.vaultTVL);
// Revert if swap reduces vault's available liquidity.
uint256 underlyingBalPost = underlying_.balanceOf(address(this));
if (underlyingBalPost <= underlyingBalPre) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (underlyingBalPost <= underlyingBalPre) {
if (underlyingBalPost < underlyingBalPre) {

Should be ok if it remains unchanged?

revert InsufficientLiquidity();
}

// sync underlying
_syncAsset(underlying_);
Expand Down Expand Up @@ -964,15 +969,4 @@ contract RolloverVault is
(uint256 trancheClaim, uint256 trancheSupply) = tranche.getTrancheCollateralization(collateralToken);
return trancheClaim.mulDiv(trancheAmt, trancheSupply, MathUpgradeable.Rounding.Up);
}

/// @dev Checks if the vault's underlying balance is above admin defined constraints.
/// - Absolute balance is strictly greater than `minUnderlyingBal`.
/// - Ratio of the balance to the vault's TVL is strictly greater than `minUnderlyingPerc`.
/// NOTE: We assume the vault TVL and the underlying to have the same base denomination.
function _enforceUnderlyingBalAfterSwap(IERC20Upgradeable underlying_, uint256 vaultTVL) private view {
uint256 underlyingBal = underlying_.balanceOf(address(this));
if (underlyingBal <= minUnderlyingBal || underlyingBal.mulDiv(ONE, vaultTVL) <= minUnderlyingPerc) {
revert InsufficientLiquidity();
}
}
}
22 changes: 20 additions & 2 deletions spot-contracts/test/rollover-vault/RolloverVault_swap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ describe("RolloverVault", function () {
);
expect(await vault.assetCount()).to.eq(2);

await collateralToken.approve(vault.address, toFixedPtAmt("1000"));
await perp.approve(vault.address, toFixedPtAmt("1000"));
await collateralToken.approve(vault.address, toFixedPtAmt("10000"));
await perp.approve(vault.address, toFixedPtAmt("10000"));
});

afterEach(async function () {
Expand Down Expand Up @@ -1260,5 +1260,23 @@ describe("RolloverVault", function () {
expect(await vault.callStatic.getTVL()).to.eq(toFixedPtAmt("4434.6153846153846152"));
});
});

describe("when vault reduces underlying liquidity", function () {
it("should be reverted", async function () {
await feePolicy.computePerpBurnFeePerc.returns(toPercFixedPtAmt("0.1"));
await feePolicy.computePerpToUnderlyingVaultSwapFeePerc.returns(toPercFixedPtAmt("0.15"));
await vault.swapPerpsForUnderlying(toFixedPtAmt("800"));

const bond = await getDepositBond(perp);
const tranches = await getTranches(bond);
await depositIntoBond(bond, toFixedPtAmt("1000"), deployer);
await tranches[0].approve(perp.address, toFixedPtAmt("200"));
await perp.deposit(tranches[0].address, toFixedPtAmt("200"));
await expect(vault.swapPerpsForUnderlying(toFixedPtAmt("1"))).to.be.revertedWithCustomError(
vault,
"InsufficientLiquidity",
);
});
});
});
});