hats-finance / Common--Stableswap-0xd4d9a2772202ce33b24901d3fc94e95a84b37430

Apache License 2.0
0 stars 0 forks source link

Changing A could allow an attacker to withdraw huge token balances when the change happens #39

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x26d1f0c46950585d0f44fa6f68fe5b97384ef99916bd9dfd68ab60461e5c2495 Severity: high

Description: Description\ Stableswap pools have an amplicifation coefficient(A) that can be adjusted depending on the peg of the stablecoins in the pool and the needed liquidity concentration. This can happen after a pool is deployed when there is not sufficient concentration or when the stablecoin peg has been changed to allow better trading.

An admin can schedule changing of A with the set_amp_coef function and can set a valid and reasonable increase or decrease of A, which could expose the pool to a loss.

Attack Scenario\ Check extremely detailed report of the vulnerabilty here - https://medium.com/@peter_4205/curve-vulnerability-report-a1d7630140ec

This issue is in scope because even an honest contract owner can make too big of changes to A and the recommended step in the article is 0.1% per block, which wouldn't be feasible to handle manually. Attachments

  1. Proof of Concept (PoC) File

Any change of A(especially downward changes) exposes the pool to a potential loss, as described here. An attack scenario would be an attacker detecing when the admin makes a change to A, sandwiching the transaction in-between in the following scenario:

  1. flashloan to imbalance the pool
  2. change A transaction
  3. swap in the reverse direction, make a profit and cause a loss to the AMM token inventory, as described in "Loss-Making Updates to A" in the referenced article

https://medium.com/@peter_4205/curve-vulnerability-report-a1d7630140ec

  1. Revised Code File (Optional) A ramping up of A, which is now implemented in StableSwap Curve contracts should be implemented to gradually change A - https://github.com/curvefi/curve-stablecoin/blob/master/contracts/Stableswap.vy#L1061
JanKuczma commented 2 months ago

Thank you for your submission.

It's a valid submission. Well done! :muscle:

aktech297 commented 2 months ago

Thank you for your submission.

It's a valid submission. Well done! 💪

We thought that the A is updated by the owner which has these safety mechanism by off chain.

DamianStraszak commented 2 months ago

After a long internal discussion we have decided to award this submission with the medium bounty. See the discussion below:

  1. Why award a bounty at all? This vulnerability is not quite typical but one could argue that if 1) the contract has an active admin, 2) the admin is not aware of this possible issue, 3) there is huge liquidity in the contract, then a specialized party could exploit the vulnerability and siphon some funds. Since we didn't include any warning about this in the documentation or in-code comments, the security researchers could assume that we were not aware of that and hence rightfully make a submission about it.

  2. Why award medium and not high? I think we all agree that there are some big assumptions to be made in order for this vulnerability to be of relevance. First of all, there must be an active admin -- something we didn't really plan for production (we will go for constant A, set at release), second the admin must be reckless in updating the paramater. Note that there are several defenses on the admin level that could be implemented: a) automating greadual updates of A, via an admin contract, b) mandatory scheduled delays for updating the parameter (this does not defend fully, but allows the LPs to withdraw if they are informed).

All in all, we estimate the likelihood of the attack to be extremely low, hence propose the medium bounty.