Basin
Findings & Analysis Report

2024-09-03

Overview
- About C4
- Wardens
Summary
Scope
Severity Criteria
High Risk Findings (2)
- [H-01] WellUpgradeable can be upgraded by anyone
- [H-02] Incorrectly assigned decimal1 parameter upon decoding
Medium Risk Findings (2)
- [M-01] For extreme ratios, getRatiosFromPriceSwap will return data for which is impossible to converge into a reserve
- [M-02] In Stable2LUT1::getRatiosFromPriceLiquidity, in extreme cases, updateReserve will start breaking
Low Risk and Non-Critical Issues
Disclosures

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Basin smart contract system written in Solidity. The audit took place between July 29 — August 8, 2024.

Following the C4 audit, an Invitational audit was conducted with a smaller subset of wardens to review the sponsor’s mitigated code. The final report including those results can be viewed here.

Wardens

39 Wardens contributed reports to Basin:

This audit was judged by 0xsomeone.

Final report assembled by thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 4 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 2 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 3 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Basin repository, and is composed of 3 smart contracts written in the Solidity programming language and includes 2414 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

Malicious Input Handling
Escalation of privileges
Arithmetic
Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (2)

[H-01] `WellUpgradeable` can be upgraded by anyone

Submitted by zanderbyte, also found by Egis_Security (1, 2), 0xSecuri (1, 2), debo, Walter (1, 2), unnamed, mgf15, Honour, 0x11singh99, Flare, ZanyBonzy, shaflow2, Mrxstrange, NoOne, John_Femi, and 0xvd

WellUpgradeable is an upgradeable version of the Well contract, inheriting from OpenZeppelin’s UUPSUpgradeable and OwnableUpgradeable contracts. According to OpenZeppelin’s documentation for UUPSUpgradeable.sol (here and here), the internal _authorizeUpgrade function must be overridden to include access restriction, typically using the onlyOwner modifier. This must be done to prevent unauthorized users from upgrading the contract to a potentially malicious implementation.

However, in the current implementation the _authorizeUpgrade function is overridden with custom logic but lacks the onlyOwner modifier. As a result, the upgradeTo and upgradeToAndCall methods can be invoked by any address, allowing anyone to upgrade the contract, leading to the deployment of malicious code and compromise the integrity of the contract.

Proof of concept

The following test demonstrates that WellUpgradeable can be upgraded by any address, not just the owner. It is based on testUpgradeToNewImplementation with the difference that a new user address is created and used to call the upgradeTo function, successfully upgrading the contract and exposing the lack of access control.

Paste the following test into WellUpgradeable.t.sol:

function testUpgradeToNewImplementationNotOwner() public {
        IERC20[] memory tokens = new IERC20[](2);
        tokens[0] = new MockToken("BEAN", "BEAN", 6);
        tokens[1] = new MockToken("WETH", "WETH", 18);
        Call memory wellFunction = Call(wellFunctionAddress, abi.encode("2"));
        Call[] memory pumps = new Call[](1);
        pumps[0] = Call(mockPumpAddress, abi.encode("2"));
        // create new mock Well Implementation:
        address wellImpl = address(new MockWellUpgradeable());
        WellUpgradeable well2 =
            encodeAndBoreWellUpgradeable(aquifer, wellImpl, tokens, wellFunction, pumps, bytes32(abi.encode("2")));
        vm.label(address(well2), "upgradeableWell2");
        
        address user = makeAddr("user");
        vm.startPrank(user);
        WellUpgradeable proxy = WellUpgradeable(payable(proxyAddress));
        proxy.upgradeTo(address(well2));

        // verify proxy was upgraded.
        assertEq(address(well2), MockWellUpgradeable(proxyAddress).getImplementation());
        vm.stopPrank();
    }

Recommended mitigation steps

Add the onlyOwner modifier to the _authorizeUpgrade function in WellUpgradeable.sol to restrict upgrade permissions:

+   function _authorizeUpgrade(address newImplmentation) internal view override onlyOwner {
        // verify the function is called through a delegatecall.
        require(address(this) != ___self, "Function must be called through delegatecall");

        // verify the function is called through an active proxy bored by an aquifer.
        address aquifer = aquifer();
        address activeProxy = IAquifer(aquifer).wellImplementation(_getImplementation());
        require(activeProxy == ___self, "Function must be called through active proxy bored by an aquifer");

        // verify the new implementation is a well bored by an aquifier.
        require(
            IAquifer(aquifer).wellImplementation(newImplmentation) != address(0),
            "New implementation must be a well implementation"
        );

        // verify the new impelmentation is a valid ERC-1967 impelmentation.
        require(
            UUPSUpgradeable(newImplmentation).proxiableUUID() == _IMPLEMENTATION_SLOT,
            "New implementation must be a valid ERC-1967 implementation"
        );
    }

Assessed type

Access Control

nickkatsios (Basin) confirmed and commented via duplicate Issue #18:

The modifier was mistakenly omitted here and should definitely be added.

0xsomeone (judge) commented:

The Warden and its duplicates have properly identified that the upgrade methodology of a WellUpgradeable is insecure, permitting any contract to be upgraded to an arbitrary implementation. Specifically, the upgrade authorization mechanism will ensure that:

The call was routed through the proxy.

The new implementation complies with the EIP-1967 standard.

The new implementation has been registered in the Aquifier.

Given that well registration on the Aquifier is unrestricted as seen here, it is possible to practically upgrade any well to any implementation.

I believe a high-risk assessment is valid as this represents a critical security issue that can directly lead to total fund loss for all wells deployed in the system.

[H-02] Incorrectly assigned `decimal1` parameter upon decoding

Submitted by ZanyBonzy, also found by SAQ, trachev, Agontuk, rare_one, Rhaydden, TheFabled, Akay, FastChecker, d4r3d3v1l, Mrxstrange, zanderbyte, macart224, Honour, Bauchibred, stuart_the_minion, johnthebaptist, willycode20, Nikki, 0xAadi, psb01, Flare, 0xRiO, 0xvd, alexzoid, KupiaSec, ArcaneAgent, and dreamcoder

Impact is high as lots of functions depend on the decodeWellData, and as such will be fed incorrect data about token1’s decimals. This can lead to potential overvaluation or undervaluation of the token’s amount and prices, depending on the context with which its used. It also ignores the fact that decimals1 can return a 0 value and as such will work with that for scaling, rather than scaling it to 18. This also depends on the decimal0 being 0;

Proof of Concept

Context: In various functions in Stable2.sol, the decodeWellData function is called to decode the passed in token decimals data, which is then used to scale the token reserves to a function of 18. This is because the tokens may have different decimals, ranging from as low as 0 to 18.

Bug Location: In the decodeWellData function, we can see that if the well data returns 0, a returned decimal of 18 is assumed as standard for the token. And as such, each decimals are scaled to that level. However, as can be seen from the @audit tag, to set a value “18” decimal1, the function incorrectly checks if decimal0 is 0, rather than checking if decimal1 is 0.

    function decodeWellData(bytes memory data) public view virtual returns (uint256[] memory decimals) {
        (uint256 decimal0, uint256 decimal1) = abi.decode(data, (uint256, uint256));

        // if well data returns 0, assume 18 decimals.
        if (decimal0 == 0) {
            decimal0 = 18;
        }
        if (decimal0 == 0) { //@audit
            decimal1 = 18;
        }
        if (decimal0 > 18 || decimal1 > 18) revert InvalidTokenDecimals();

        decimals = new uint256[](2);
        decimals[0] = decimal0;
        decimals[1] = decimal1;
    }

Final Effect: This means that regardless of the value returned for decimal1 from the decoding, it’s replaced with 18, even if token1 is a token with 6 decimals, or less than 18. As a result, the reserves will be potentially scaled with a decimal different from actual. Also, when decimal1 is 0, rather than scaling to a value of 18, it ignores this value and attempts to work with a value of 0 instead. As the function is used extensively, in the codebase, this can lead to serious price miscalculations.

The functions are listed below:

Runnable POC: The gist link here contains a test case that shows that the calcLpTokenSupply function (using it as our example) breaks when decimal1 data returns 0, since its wrongly decoded. Based on the expectation, the test should pass, but it doesn’t. It reverts with the error shown below:

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] test_calcLpTokenSupply_sameDecimals2() (gas: 61838)
Traces:
  [61838] Stable2Test::test_calcLpTokenSupply_sameDecimals2()
    ├─ [4095] Stable2::calcLpTokenSupply([10000000000000000000 [1e19], 10000000000000000000 [1e19]], 0x00000000000000000000000000000000000000000000000000000000000000120000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.01ms (98.33µs CPU time)

Ran 1 test suite in 151.81ms (1.01ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/functions/Stable2.t.sol:Stable2Test
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] test_calcLpTokenSupply_sameDecimals2() (gas: 61838)

Encountered a total of 1 failing tests, 0 tests succeeded

Recommended Mitigation Steps

Change the check:

//...
-        if (decimal0 == 0) {
+        if (decimal1 == 0) {
            decimal1 = 18;
//...
        }

Assessed type

en/de-code

nickkatsios (Basin) confirmed and commented:

This is valid and needs to be corrected. Good catch!

0xsomeone (judge) increased severity to High and commented:

The Warden has outlined an issue in the way decimals are handled when decoding immutable well data and specifically a problem that will arise when the decimal0 configuration is 0 whilst the decimal1 configuration is a value other than 18 and 0.

I believe that a high-risk rating is better suited for this submission as the decimals utilized throughout the swapping calculations are imperative to the AMM’s safe operation.

Medium Risk Findings (2)

[M-01] For extreme ratios, `getRatiosFromPriceSwap` will return data for which is impossible to converge into a reserve

Submitted by Egis_Security

The 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. In such situations, the function will return the following struct:

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

If you 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. 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

Recommended Mitigation Steps

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

Assessed type

Invalid Validation

Brean0 (Basin) confirmed

0xsomeone (judge) decreased severity to Low and commented:

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.

nmirchev8 (warden) commented:

@0xsomeone - Here are my arguments regarding why the issue should be a valid Medium:

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.

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.

Here is a proof from sponsor that currently there is no production environment for the corresponding contracts:

Note: to view the provided image, please see the original submission here.

nickkatsios (Basin) commented:

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.

0xsomeone (judge) increased severity to Medium and commented:

Hey @nmirchev8 and @nickkatsios - 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.

[M-02] In `Stable2LUT1::getRatiosFromPriceLiquidity`, in extreme cases, `updateReserve` will start breaking

Submitted by Egis_Security

This is one of the edge cases in getRatiosFromPriceLiquidity:

if (price < 0.001083e6) {
                                    revert("LUT: Invalid price");
                                } else {
                                    return
                                        PriceData(
                                            0.27702e6, 
                                            0, 
                                            9.646293093274934449e18, 
                                            0.001083e6, 
                                            0, 
                                            2000e18, 
                                            1e18);
                                }

The range where the price can be is very large, between 0.27702e6 (highPrice) and 0.001083e6 (lowPrice). If we are closer to the lowPrice, then pd.currentPrice is set to pd.lutData.lowPrice:

if (pd.lutData.highPrice - pd.targetPrice > pd.targetPrice - pd.lutData.lowPrice) {
            // targetPrice is closer to lowPrice.
            scaledReserves[j] = scaledReserves[i] * pd.lutData.lowPriceJ / pd.lutData.precision;

            // set current price to lowPrice.
            pd.currentPrice = pd.lutData.lowPrice;
        }

In this case, the current price is much smaller than the target price and because of this, updateReserve will have to do a very large correction of the reserve in order to converge on the target price.

Because of these large corrections, the reserve will become so small, that the next time the reserve has to be updated, the amount that it has to be reduced by will be larger than the reserve itself, resulting in a panic underflow and bricking the function.

This revert happens because of two reasons:

Because of the large range that the price can be in, thus using such a small lowPrice as pd.currentPrice makes the difference between pd.targetPrice so large that the corrections become very large and underflow before convergence can be achieved.
lowPriceJ is extremely large. In this case it’s 2000e18, which makes the step size very large and thus, the code will attempt extreme corrections of the reserve, resulting in a revert.

It’s important to note, that even if lowPriceJ is significantly reduced, the function won’t revert, but it will never converge on a price and this can only be fixed by reducing the gap between highPrice and lowPrice effectively reducing the range for that specific price.

We wanted to showcase both issues and that even if the underflow (lowPriceJ issue) is resolved that the estimated ranges still are too large, thus making the price impossible to converge.

The below tests showcase both the original code (underflow) and how reducing lowPriceJ doesn’t fix the real problem.

Proof of Concept

Test case for underflow: paste the following inside BeanstalkStable2LiquidityTest and run forge test --match-contract BeanstalkStable2LiquidityTest --mt test_calcReserveAtRatioLiquiditExtreme -vvvv:

function test_calcReserveAtRatioLiquiditExtreme() public view {
        uint256[] memory reserves = new uint256[](2);
        reserves[0] = 1e18;
        reserves[1] = 1e18;
        uint256[] memory ratios = new uint256[](2);
        ratios[0] = 8;
        ratios[1] = 1;

        uint256 reserve0 = _f.calcReserveAtRatioLiquidity(reserves, 0, ratios, data);
    }

Logs:

  Target Price:  125000
  Original Current Price:  1083
  --------------------------------
  Reserve before step:  2000000000000000000000
  --------------------------------
  Reserve:                2000000000000000000000
  Amount to decrease by:  893822359084720968727
  Current Price at iteration  0 1993
  Reserve:                1106177640915279031273
  Amount to decrease by:  887258462712414537152
  Current Price at iteration  1 10817
  Reserve:                218919178202864494121
  Amount to decrease by:  823610307119851952292

As you can see, the code attempts massive corrections to the reserve in order to achieve convergence, but the amount to decrease the reserve is too large and the function underflows.

Test case when lowering lowPriceJ: run the same test as before, but change lowPriceJ to 10e18, for example purposes:

if (price < 0.001083e6) {
                                    revert("LUT: Invalid price");
                                } else {
                                    return
                                        PriceData(
                                            0.27702e6, 
                                            0, 
                                            9.646293093274934449e18, 
                                            0.001083e6, 
                                            0, 
                                      ->    10e18, 
                                            1e18);
                                }

Logs:

Target Price:  125000
  Original Current Price:  1083
  --------------------------------
  Reserve before step:  10000000000000000000
  --------------------------------
  Reserve:                10000000000000000000
  Amount to decrease by:  158841687633952488
  Current Price at iteration  0 272070
  ...
  Reserve:                20736004517896372313
  Amount to increase by:  8588323693661738
  Current Price at iteration  254 131644
  END OF NEWTON's METHOD

The logs are compacted as they are very large, but the idea is this:

Target price is again much larger than current price.
During the first correction, the price moves by a very large amount, making it much larger than target price.
Now updateReserves has to do corrections in the other direction (increase the reserve), so that the prices can converge on the target price, but this never happens, as Newton’s method ends after 255 iterations and leaves the loop.
calcReserveAtRatioLiquidity will return 0, as reserve is never initialized anywhere, effectively making the function useless.

Tools Used

Foundry

Recommended Mitigation Steps

To completely fix both issues, lowPriceJ must be lowered and the estimated range must be narrowed down.

Assessed type

DoS

Brean0 (Basin) confirmed

0xsomeone (judge) decreased severity to Low and commented:

The Warden outlines a misbehavior that arises from the edge case price configuration outlined in Issue #25. I consider both issues to stem from the extreme difference between the upper and lower price bounds, as eliminating this gap would resolve both issues and thus be considered the root cause of both.

This particular submission once again relies on direct interaction with the Stable2LUT1 contract and does not adequately demonstrate the issue in a production environment. A demonstration of either this one or exhibit #25 will be considered acceptable to consider them acceptable per my comment on #25.

deth (warden) commented:

@0xsomeone - Here are our arguments why this is a valid Medium and it is not a duplicate of #25:

Why this is valid:

The case may be an edge case, but it is in the Stable2LUT1 and must be considered.

The PoC provided is the only one that can be provided as of now, as currently Beanstalk (the integrator of the function) doesn’t have any production ready code built around the Stable2 contract (the caller of getRatiosFromPriceLiquidity). Thus, we cannot provide a production ready PoC since there is nothing to base it off of.

The issue should be considered a Medium, as it breaks core functionality completely, making any call to getRatiosFromPriceLiquidity unusable, which can have a large impact on all integrators that use/will use the function. Thus, with a low likelihood and High impact, we believe this a valid Medium.

Why this isn’t a duplicate:

The root between the two issues are different. One occurs in getRatiosFromPriceLiquidity and the other in getRatiosFromPriceSwap. If #25 is fixed this issue will still persist and vice-versa.

nickkatsios (Basin) commented:

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, both issues were 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.

0xsomeone (judge) increased severity to Medium and commented:

See response in the primary submission Issue #25.

deth (warden) commented:

@0xsomeone - I don’t understand, how is the root cause is the same? If #25 is fixed, #22 persists and vice-versa. I understand that main driving force behind both issues is the same, but fixing one of them doesn’t fix the other, you can see the sponsors applied fixes to both functions independently.

0xsomeone (judge) commented:

@deth - While discussing with the Sponsor, we concluded that both issues stem from the extreme step sizes on the lower bounds of the respective curves. However, you are indeed correct in that the actual ratios are yielded by distinct functions and thus there is no actual correlation between the two submissions (i.e., it is not possible to fix both simultaneously with a single code change).

As such, I have proceeded with splitting this submission as a unique entry given that from a C4 perspective the issues do not strictly share a root cause. I greatly appreciate the due diligence!

Low Risk and Non-Critical Issues

For this audit, 3 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by Rhaydden received the top score from the judge.

The following wardens also submitted reports: ZanyBonzy and 0xAadi.

[01] Potential non-convergence in `calcLpTokenSupply` function could lead to silent failures

The calcLpTokenSupply function may not always converge within the 255 iterations limit. If it doesn’t converge, the function will silently exit the loop without returning a value or throwing an error.

Proof of Concept

Take a look at the calcLpTokenSupply function: https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/functions/Stable2.sol#L74-L103

function calcLpTokenSupply(
    uint256[] memory reserves,
    bytes memory data
) public view returns (uint256 lpTokenSupply) {
    if (reserves[0] == 0 && reserves[1] == 0) return 0;
    uint256[] memory decimals = decodeWellData(data);
    // scale reserves to 18 decimals.
    uint256[] memory scaledReserves = getScaledReserves(reserves, decimals);

    uint256 Ann = a * N * N;

    uint256 sumReserves = scaledReserves[0] + scaledReserves[1];
    if (sumReserves == 0) return 0;
    lpTokenSupply = sumReserves;
    for (uint256 i = 0; i < 255; i++) {
        uint256 dP = lpTokenSupply;
        // If division by 0, this will be borked: only withdrawal will work. And that is good
        dP = dP * lpTokenSupply / (scaledReserves[0] * N);
        dP = dP * lpTokenSupply / (scaledReserves[1] * N);
        uint256 prevReserves = lpTokenSupply;
        lpTokenSupply = (Ann * sumReserves / A_PRECISION + (dP * N)) * lpTokenSupply
            / (((Ann - A_PRECISION) * lpTokenSupply / A_PRECISION) + ((N + 1) * dP));
        // Equality with the precision of 1
        if (lpTokenSupply > prevReserves) {
            if (lpTokenSupply - prevReserves <= 1) return lpTokenSupply;
        } else {
            if (prevReserves - lpTokenSupply <= 1) return lpTokenSupply;
        }
    }
}

The issue here is that if the loop completes without finding a convergent solution, the function will exit without returning a value, leading to silent failures.

Recommended Mitigation Steps

Consider adding a revert statement after the loop to ensure that if the calculation doesn’t converge within 255 iterations; the function will revert with a clear error message. Here is the recommended change in diff format:

        if (lpTokenSupply > prevReserves) {
            if (lpTokenSupply - prevReserves <= 1) return lpTokenSupply;
        } else {
            if (prevReserves - lpTokenSupply <= 1) return lpTokenSupply;
        }
    }
+   revert("LP token supply calculation did not converge");
}

[02] Non-adherence to EIP-1822 standard in `WellUpgradeable.sol`

The WellUpgradeable contract fails to meet the EIP-1822 standard, potentially causing issues with upgradeability and compatibility with proxy contracts. It is explicitly mentioned in the ReadMe that src/WellUpgradeable.sol should adhere to EIP-1822.

Proof of Concept

Issue 1: Absence of updateCodeAddress Functionality:

According to the EIP-1822 standard:

⚠️ Warning: updateCodeAddress and proxiable must be present in the Logic Contract. Failure to include these may prevent upgrades and could allow the Proxy Contract to become entirely unusable. See below Restricting dangerous functions.

WellUpgradeable.sol, which serves as a logic contract, lacks the updateCodeAddress function, which is necessary to store the Logic Contract’s address at the specified storage position.

Issue 2: Incorrect proxiableUUID Implementation:

EIP-1822 mandates that the proxiableUUID function should return a specific bytes32 value derived from keccak256("PROXIABLE"). However, WellUpgradeable returns _IMPLEMENTATION_SLOT instead.

Here is the correct implementation as per the EIP-1822 Standard:

function proxiableUUID() public pure returns (bytes32) {
    return 0xc5f16f0fcc639fa48a6947836d9850f504798523bf8c9a3a87d5876cf622bcf7;
}

Here is the incorrect implementation in WellUpgradeable:

function proxiableUUID() external view override notDelegatedOrIsMinimalProxy returns (bytes32) {
    return _IMPLEMENTATION_SLOT;
}

Issue 3: Deviation from Constructor Pattern:

EIP-1822 recommends a flexible constructor pattern for initializing the logic contract. WellUpgradeable does not follow this pattern.

Standard excerpt:

constructor(bytes memory constructData, address contractLogic) public {
    // Initialization logic
}

But in WellUpgradeable:

function init(string memory _name, string memory _symbol) external override reinitializer(2) {
    // Initialization logic
}

Recommended Mitigation Steps

Implement the updateCodeAddress function to store the Logic Contract’s address at the specified storage position.
Modify the proxiableUUID function to return the specific bytes32 value as required by EIP-1822, ensuring compliance with the standard’s expectations for identifying proxiable contracts.
Adjust the initialization process of WellUpgradeable to align with the constructor pattern recommended by EIP-1822.

[03] Misspelled variable name in Upgrade Authorization Logic

The misspelling of the variable newImplementation as newImplmentation in the _authorizeUpgrade function prevents the contract from compiling successfully. This issue impedes the upgrade mechanism of the contract, potentially locking it in an outdated state.

Proof of Concept

In the _authorizeUpgrade function, the following line attempts to verify that the new implementation address is a well implementation bored by an aquifer.

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L76

require(
    IAquifer(aquifer).wellImplementation(newImplmentation) != address(0),
    "New implementation must be a well implementation"
);

However, the variable newImplmentation is misspelled. The correct variable name, as passed to the function, is newImplementation.

Recommended Mitigation Steps

Correct the spelling of the variable newImplementation in the _authorizeUpgrade function to match the parameter name and also correct all the instances in the contract.

[04] Parity reserve calculation in `calcReserveAtRatioSwap` could lead to suboptimal pricing in some edge cases

The calcReserveAtRatioSwap function uses a simplified calculation for the parity reserve (lpTokenSupply / 2). This assumption may not hold true for stableswap pools, especially when the tokens have different precisions or when the pool is imbalanced. This can lead to suboptimal pricing calculations, particularly in edge cases or when the pool is far from balanced.

Proof of Concept

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/functions/Stable2.sol#L173-L239

// calculate lp token supply:
uint256 lpTokenSupply = calcLpTokenSupply(scaledReserves, abi.encode(18, 18));

// lpTokenSupply / 2 gives the reserves at parity:
uint256 parityReserve = lpTokenSupply / 2;

// update `scaledReserves` based on whether targetPrice is closer to low or high price:
if (pd.lutData.highPrice - pd.targetPrice > pd.targetPrice - pd.lutData.lowPrice) {
    // targetPrice is closer to lowPrice.
    scaledReserves[i] = parityReserve * pd.lutData.lowPriceI / pd.lutData.precision;
    scaledReserves[j] = parityReserve * pd.lutData.lowPriceJ / pd.lutData.precision;
    // initialize currentPrice:
    pd.currentPrice = pd.lutData.lowPrice;
} else {
    // targetPrice is closer to highPrice.
    scaledReserves[i] = parityReserve * pd.lutData.highPriceI / pd.lutData.precision;
    scaledReserves[j] = parityReserve * pd.lutData.highPriceJ / pd.lutData.precision;
    // initialize currentPrice:
    pd.currentPrice = pd.lutData.highPrice;
}

Recommended Mitigation Steps

Replace the simplified calculation of parityReserve with a more accurate calculation that considers the amplification factor (A) and the specific dynamics of the pool. Implement a function to calculate the true parity reserve based on the current state of the pool and the amplification factor, and use this function in place of lpTokenSupply / 2.

[05] Lack of check for identical implementation during upgrade in `_authorizeUpgrade` function

The absence of a check to ensure that the new implementation is different from the current implementation could lead to unnecessary upgrades. This might result in wasted gas fees and potential confusion during the upgrade process.

Proof of Concept

The _authorizeUpgrade function currently verifies that the new implementation is a valid well implementation bored by an aquifer and a valid ERC-1967 implementation. However, it does not check if the new implementation is different from the current implementation.

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L65-L85

function _authorizeUpgrade(address newImplementation) internal view override {
    // verify the function is called through a delegatecall.
    require(address(this) != ___self, "Function must be called through delegatecall");

    // verify the function is called through an active proxy bored by an aquifer.
    address aquifer = aquifer();
    address activeProxy = IAquifer(aquifer).wellImplementation(_getImplementation());
    require(activeProxy == ___self, "Function must be called through active proxy bored by an aquifer");

    // verify the new implementation is a well bored by an aquifer.
    require(
        IAquifer(aquifer).wellImplementation(newImplementation) != address(0),
        "New implementation must be a well implementation"
    );

    // verify the new implementation is a valid ERC-1967 implementation.
    require(
        UUPSUpgradeable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT,
        "New implementation must be a valid ERC-1967 implementation"
    );
}

Recommended Mitigation Steps

Add a check to ensure that the new implementation is different from the current implementation.

Brean0 (Basin) commented:

[01] - Agreed.
[02] - This report is correct given that the audit requested that the well upgradeable adhere to EIP-1822, but since then, the Basin Development Community has agreed that the benefits of deviating from the standard outweigh the benefits.
[03] - Agreed.
[04] - By definition, the LP token supply is the summation of the reserves when the reserves are equal. This can be seen in the stableswap invariant. At most, this may be off by 1 unit due to division error, which is acceptable. Without a test to validate this claim it’s unclear how to proceed.
[05] - It is up to the upgrader to ensure that the new implementation is different. Damage here is limited to the upgrader.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.

Basin Findings & Analysis Report

2024-09-03

Table of contents

Overview

About C4

Wardens

Summary

Scope

Severity Criteria

High Risk Findings (2)

[H-01] WellUpgradeable can be upgraded by anyone

Proof of concept

Recommended mitigation steps

Assessed type

[H-02] Incorrectly assigned decimal1 parameter upon decoding

Proof of Concept

Recommended Mitigation Steps

Assessed type

Medium Risk Findings (2)

[M-01] For extreme ratios, getRatiosFromPriceSwap will return data for which is impossible to converge into a reserve

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

[M-02] In Stable2LUT1::getRatiosFromPriceLiquidity, in extreme cases, updateReserve will start breaking

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

Low Risk and Non-Critical Issues

[01] Potential non-convergence in calcLpTokenSupply function could lead to silent failures

Proof of Concept

Recommended Mitigation Steps

[02] Non-adherence to EIP-1822 standard in WellUpgradeable.sol

Proof of Concept

Recommended Mitigation Steps

[03] Misspelled variable name in Upgrade Authorization Logic

Proof of Concept

Recommended Mitigation Steps

[04] Parity reserve calculation in calcReserveAtRatioSwap could lead to suboptimal pricing in some edge cases

Proof of Concept

Recommended Mitigation Steps

[05] Lack of check for identical implementation during upgrade in _authorizeUpgrade function

Proof of Concept

Recommended Mitigation Steps

Disclosures

Basin
Findings & Analysis Report

[H-01] `WellUpgradeable` can be upgraded by anyone

[H-02] Incorrectly assigned `decimal1` parameter upon decoding

[M-01] For extreme ratios, `getRatiosFromPriceSwap` will return data for which is impossible to converge into a reserve

[M-02] In `Stable2LUT1::getRatiosFromPriceLiquidity`, in extreme cases, `updateReserve` will start breaking

[01] Potential non-convergence in `calcLpTokenSupply` function could lead to silent failures

[02] Non-adherence to EIP-1822 standard in `WellUpgradeable.sol`

[04] Parity reserve calculation in `calcReserveAtRatioSwap` could lead to suboptimal pricing in some edge cases

[05] Lack of check for identical implementation during upgrade in `_authorizeUpgrade` function