For extreme ratios getRatiosFromPriceSwap will return data for which is impossible to converge into a reserve

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/functions/StableLUT/Stable2LUT1.sol#L750-L758

Vulnerability details

Impact

Basin team has implemented look up table to fetch pre-calculated values, which are close to the targetPrice to decrease the complexity of calcReserveAtRatioSwap and calcReserveAtRatioLiquidity functions. Stable2LUT1 has a function getRatiosFromPriceSwap, which returns PriceData struct based on provided price. We can see that in case of super extreme price, the function will revert with LUT: Invalid price, but there is also support for extreme prices, which we assume should work correctly, when such situation occurs.

We will investigate an edge case, which is present, when we enter if (price < 0.213318e6) and price is above 0.001083e6. Is such situation, function will return the following struct:

PriceData(
0.213318e6, // highPrice
0.188693329162796575e18,
2.410556040105746423e18,
0.001083e6, // lowPrice                       
0.005263157894736842e18,                      
10.522774272309483479e18,
1e18
);

If you can notice we have a large gap between highPrice and lowPrice and more precisely.

The following results in large jump in reserves calculation when updateReserve which leads to skipping the target price sequentially, which moves away pd.currentPrice until we exit the for loop, which returns 0.

Here is an estimation of the impact based on the PoC which is below.: For a ratio ~ 1:4.6 of the price of the tokens

• We have a targetPrice of 212104 (0.21)
• We end up with reserves for a price 44957 (0.04), which is ~ X5 less than the requested amount
• As result we receive 3623929482258792273, instead of 2421185918213441156, which is a big difference
• NOTE that we don't return the compromised data, but this is the calculated value on the last iteration

Proof of Concept

Place the following test inside /test/beanstalk/BeanstalkStable2.calcReserveAtRatioLiquidity.t.sol and run it with forge test --mt "test_calcReserveAtRatioSwapSkipTarget" -vv

    function test_calcReserveAtRatioSwapSkipTarget() public view {
        uint256[] memory reserves = new uint256[](2);
        reserves[0] = 1e18;
        reserves[1] = 1e18;
        uint256[] memory ratios = new uint256[](2);
        ratios[0] = 4202;
        ratios[1] = 19811;

        // 4202 * 1e6 / 19811  = 212104 = 0.212 / 1  = 1 : 4.6
        //                                0.04  / 1  = 1 : 25    

        uint256 reserve1 = _f.calcReserveAtRatioSwap(reserves, 1, ratios, data);
        //console.log("Reserves 1 :", reserve1);
    }

You can further add console.log inside the for loop of calcReserveAtRatioSwap and log pd.currentPrice on each update. You can notice that each time the current price is further away from the target. Here are logs:

Logs:
  We want  212104
  Lud Data High:  213318
  Lud Data Low:  1083
  Max step size:  1858346112180059079
  Scaled reserves when closest J:  2421185918213441156
  Scaled reserves when closest I:  188693329162796575
  Rate at iteration  0  is  209986
  Rate at iteration  1  is  215834
  Rate at iteration  2  is  205646
  Rate at iteration  3  is  223618
  Rate at iteration  4  is  192647
  Rate at iteration  5  is  247954
  Rate at iteration  6  is  156345
  Rate at iteration  7  is  320536
  Rate at iteration  8  is  83844
  Rate at iteration  9  is  409344
  Rate at iteration  10  is  42030
  Rate at iteration  11  is  291810
  Rate at iteration  12  is  106911
  Rate at iteration  13  is  401234
  Rate at iteration  14  is  44576
  Rate at iteration  15  is  306730
  Rate at iteration  16  is  94144
  Rate at iteration  17  is  409601
  Rate at iteration  18  is  41952
  Rate at iteration  19  is  291337
  Rate at iteration  20  is  107346
  Rate at iteration  21  is  400812
...

Tools Used

• Fuzz Testing
• Manual Review

Recommended Mitigation Steps

Recalculate the PriceData for the given case and consider narrowing down the price diff.

Assessed type

Invalid Validation

[alex-ppg]

The Warden has identified a potential edge case in the pre-computed values yielded by the Stable2LUT1::getRatiosFromPriceSwap function where the discrepancy between the upper and lower price bounds is significant and adversely affects reserve calculations.

I believe a medium-risk assessment is better suited for this particular vulnerability due to its low likelihood of manifesting in a production environment. Specifically, the PoC will directly interact with the Stable2 contract even though the ratios passed into it stem from different calculations in the MultiFlowPump contract.

In order to properly accept the medium-risk assessment, I invite the wardens of the team to supplement a PoC that demonstrates the vulnerability manifesting in a production environment through a higher-level call to the Basin system. As the issue stands, it lacks sufficient impact although the actual flaw is adequately described.

For the above reason, I will consider this submission as QA (L) pending substantiation by the Warden team.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c

[NicolaMirchev]

Hey @alex-ppg

Thanks for your expertise about the following finding. Here are my arguments regarding why the issue should be a valid Medium:

1. We can both agree that the scope of this competition is very abstract and 90% of the functions are "view". As auditors, we should ensure that the code in scope behaves as expected. The following issue is bound in the limited scope of this competition and can be summarized as "break core functionality". When the scope is abstract, we should assume that every case in the provided code may be executed.
2. The only precondition for the vulnerability to occur is to have a ratio of ~ 1:4.6, which is realistic in de-peg situations for stablecoins.
3. Here is a proof from sponsor that currently there is no production environment for the corresponding contracts:

[nickkatsios]

We would tend to agree with classifying this issue as Medium severity. While the likelihood of it occurring in a real production environment is low, it effectively renders the well function unusable. According to the severity documentation, which states that "assets are not at direct risk, but the protocol's function or availability could be impacted," this issue clearly falls within that category. Instead of recalculating the data points for the lookup table, the issue was resolved by improving the step size, ensuring that the maxStep size can never exceed the j reserve. You can see the complete fix in this PR: https://github.com/BeanstalkFarms/Basin/pull/137.

[alex-ppg]

Hey @NicolaMirchev and @nickkatsios, thank you for your PJQA contributions! After evaluating all relevant information as well as discussing it with the Sponsor, I believe that the submission should be accepted as a medium-risk vulnerability due to relying on hypotheticals yet demonstrating the flaw clearly.

Submission #22 was resolved using the same approach as this one and thus the root cause is the large gap that exists in the step size. As such, the issues will remain group rendering this submission to be treated as a "single" one.

[c4-judge]

This previously downgraded issue has been upgraded by alex-ppg

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory