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

Min deployment amount enforcement #150

Merged
merged 8 commits into from
May 8, 2023
Merged

Min deployment amount enforcement #150

merged 8 commits into from
May 8, 2023

Conversation

aalavandhan
Copy link
Member

@aalavandhan aalavandhan commented May 2, 2023

  • Enforcing a minimum amount to be deployed
  • Guards against potential grief from dust tokens which could trigger recoverAndRedeploy
  • Contract owner can control minDeploymentAmt param

@aalavandhan aalavandhan requested review from brandoniles and nms-7 May 2, 2023 18:50
Copy link
Member

@brandoniles brandoniles left a comment

Choose a reason for hiding this comment

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

Nice that we can reuse that same existing logic 👍

Might seem a bit weird that it's a minimum amount of underlying asset, but denominated in perps?

spot-contracts/contracts/vaults/RolloverVault.sol Outdated Show resolved Hide resolved
spot-contracts/contracts/vaults/RolloverVault.sol Outdated Show resolved Hide resolved
spot-contracts/contracts/vaults/RolloverVault.sol Outdated Show resolved Hide resolved
Co-authored-by: Brandon Iles <brandon@fragments.org>
@aalavandhan
Copy link
Member Author

Nice that we can reuse that same existing logic 👍

Might seem a bit weird that it's a minimum amount of underlying asset, but denominated in perps?

Good catch. Updating to check the underlying amounts tranched ..

@aalavandhan aalavandhan force-pushed the min-undeployed branch 2 times, most recently from 1b6b102 to 4a39197 Compare May 4, 2023 21:55
spot-contracts/contracts/vaults/RolloverVault.sol Outdated Show resolved Hide resolved
revert NoDeployment();
(uint256 deployedAmt, TrancheData memory td) = _tranche(perp.getDepositBond());
uint256 perpsRolledOver = _rollover(perp, td);
if (deployedAmt <= minDeploymentAmt || perpsRolledOver <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to check perpsRolledOver <= 0?

I think this would only be the case if we roll over a small enough dust amount of underlying such that perps rounds to 0. Would we still want to revert in that case? I think we'd want it to go through.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not having this check, opens up a grief attack when external callers can just tranche the underlying and the batch job has to pay for the gas to untranche (and or retranche into a new bond).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice catch. Basically enforces that we only tranche the underlying if it can immediately be used for rotations.

Might be worth a code comment

Co-authored-by: Brandon Iles <brandon@fragments.org>
Copy link
Member

@brandoniles brandoniles left a comment

Choose a reason for hiding this comment

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

LGTM

revert NoDeployment();
(uint256 deployedAmt, TrancheData memory td) = _tranche(perp.getDepositBond());
uint256 perpsRolledOver = _rollover(perp, td);
if (deployedAmt <= minDeploymentAmt || perpsRolledOver <= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice catch. Basically enforces that we only tranche the underlying if it can immediately be used for rotations.

Might be worth a code comment

@aalavandhan aalavandhan merged commit 6b14825 into main May 8, 2023
@aalavandhan aalavandhan deleted the min-undeployed branch May 8, 2023 15:13
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.

2 participants