GalloDaSballo
commented
2 years ago 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
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.