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

How much to approve before calling mintTo #86

Open
code423n4 opened this issue Oct 10, 2021 · 1 comment
Open

How much to approve before calling mintTo #86

code423n4 opened this issue Oct 10, 2021 · 1 comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Handle

pauliax

Vulnerability details

Impact

I see one inconvenience with function mintTo and handleFees. Because handleFees changes ibRatio, and later pullUnderlying is invoked, as a user I may not know for how much amount of tokens should I approve Basket contract as this new recalculated ibRatio may change tokenAmount. Usually, it is a good security practice not to give infinite approvals, but because handleFees uses block.timestamp which cannot be reliably predicted and depends on the network congestion, I assume it would be hard to precalculate the exact amounts to approve for pullUnderlying.

Recommended Mitigation Steps

I am not sure what's the best solution here, maybe handleFees can be extracted as a standalone function that can be invoked anytime by anyone, and then do not call it from other functions if that works in your situation. Or fees can be handled afterward but then it may impact other parts. So submitting this FYI, maybe you will find this not relevant.

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding labels Oct 10, 2021
code423n4 added a commit that referenced this issue Oct 10, 2021
@frank-beard frank-beard added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Oct 19, 2021
@GalloDaSballo
Copy link
Collaborator

Agree with finding, options could be:

  • Add a slippage check (max amount to be pulled)
  • Change the math to use the amount to be pulled rather than the shares to mint

Or alternatively, to save gas, and because I don't believe it causes too many issues, you may decide to have the share stream done separately and not on every mint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Warden finding sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants