Basin - oakcobalt's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/Well.sol#L352-L358 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/Well.sol#L645

Vulnerability details

Impact

In Well.sol, all token trading and liquidity changing operations are invoked with _updatePumps() hook to pass reserve change to the Pump, except for reserve changes by calling shift().

As a result, any reserve modification from shift() will not be updated in any pumps which easily creates a deep discrepancy between the well and the pump.

Because under normal conditions, any reserve changes from shift() will not be picked up by any pumps and this can also be easily exploited, I evaluate this to be High in severity.

Proof of Concept

The intended behavior is to update the pumps with any operation that modifies the Well’s reserves. This implemented in _swapFrom(),swapTo(),_addLiquidity(), removeLiquidity etc. In these functions, _updatePumps() hook will be called, which pass current token reserves to the pumps through 'update()`.

//Well.sol
    function _swapFrom(
        IERC20 fromToken,
        IERC20 toToken,
        uint256 amountIn,
        uint256 minAmountOut,
        address recipient
    ) internal returns (uint256 amountOut) {
        IERC20[] memory _tokens = tokens();
|>        uint256[] memory reserves = _updatePumps(_tokens.length);
...

//Well.sol
    function _updatePumps(
        uint256 _numberOfTokens
    ) internal returns (uint256[] memory reserves) {
...
|>          try IPump(_pump.target).update(reserves, _pump.data) {} catch {}
...

However, shift() which also allows users to trade and modify token reserves doesn't implement _updatePumps() or other mechanisms to update reserves to the pumps. This creates an easy bypass for token reserve changes not picked up by any pumps, which can be exploited by malicious uers.

//Well.sol
    function shift(
        IERC20 tokenOut,
        uint256 minAmountOut,
        address recipient
    ) external nonReentrant returns (uint256 amountOut) {
        IERC20[] memory _tokens = tokens();
        uint256[] memory reserves = new uint256[](_tokens.length);
        // Use the balances of the pool instead of the stored reserves.
        // If there is a change in token balances relative to the currently
        // stored reserves, the extra tokens can be shifted into `tokenOut`.
        for (uint256 i; i < _tokens.length; ++i) {
            reserves[i] = _tokens[i].balanceOf(address(this));
        }
        uint256 j = _getJ(_tokens, tokenOut);
        amountOut =
            reserves[j] -
            _calcReserve(wellFunction(), reserves, j, totalSupply());
        if (amountOut >= minAmountOut) {
            tokenOut.safeTransfer(recipient, amountOut);
            reserves[j] -= amountOut;
            _setReserves(_tokens, reserves);
            emit Shift(reserves, tokenOut, amountOut, recipient);
        } else {
            revert SlippageOut(amountOut, minAmountOut);
        }
    }

Tools Used

Manual Vscode

Recommended Mitigation Steps

use _updatePumps() for shift() as well since it also modifies the reserve.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/pumps/MultiFlowPump.sol#L36-L37 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/pumps/MultiFlowPump.sol#L205-L208 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/pumps/MultiFlowPump.sol#L212-L215

Vulnerability details

Impact

When multiple tokens are handled in MultiFlowPump.sol, during trading, the pump might either unfairly reflects volatile assets to be more stale than normal trading activities, Or may fail to have any effects on relatively more stable assets.

This could distort SMA and EMA token reserve values and unfairly reflect some token prices because it uses a one-size-fits-all approach to multiple token asset prices indiscriminate of their normal volatility levels.

Proof of Concept

In MultiFlowPump.sol, a universal cap of maximum percentage change of a token reserve is set up as immutable variables - LOG_MAX_INCREASE and LOG_MAX_DECREASE. These two values are benchmarks for all tokens handled by the pump and are used to cap token reserve change per block in _capReserve(). And this cap is applied and checked on every single update() invoked by Well.sol.

//MultiFlowPump.sol
    bytes16 immutable LOG_MAX_INCREASE;
    bytes16 immutable LOG_MAX_DECREASE;

//MultiFlowPump.sol-update()
...
  for (uint256 i; i < numberOfReserves; ++i) {
            // Use a minimum of 1 for reserve. Geometric means will be set to 0 if a reserve is 0.
|>            pumpState.lastReserves[i] = _capReserve(
                pumpState.lastReserves[i], (reserves[i] > 0 ? reserves[i] : 1).fromUIntToLog2(), blocksPassed
            );
...

//MultiFlowPump.sol-_capReserve()
...
        if (lastReserve.cmp(reserve) == 1) {
|>            bytes16 minReserve = lastReserve.add(blocksPassed.mul(LOG_MAX_DECREASE));
            // if reserve < minimum reserve, set reserve to minimum reserve
            if (minReserve.cmp(reserve) == 1) reserve = minReserve;
        }
        // Rerserve Increasing or staying the same.
        else {
|>            bytes16 maxReserve = blocksPassed.mul(LOG_MAX_INCREASE);
            maxReserve = lastReserve.add(maxReserve);
            // If reserve > maximum reserve, set reserve to maximum reserve
            if (reserve.cmp(maxReserve) == 1) reserve = maxReserve;
        }
        cappedReserve = reserve;
...

As seen from above, whenever a token reserve change is over the percentage dictated by LOG_MAX_INCREASE or LOG_MAX_DECREASE, the token reserve change will be capped at the maximum percentage and rewritten as calculated minReserve or maxReserve value. This is fine if the pump is only managing one trading pair(two tokens) because their trading volatility level can be determined.

However, this is highly vulnerable when multiple trading pairs and multiple token assets are handled by a pump. And this is the intended scenario, which can be seen in Well.sol _updatePumps() where all token reserves handled by well will be passed to MultiFlowPump.sol. In this case, either volatile tokens will become more stale compared to their normal trading activities, or some stable tokens are allowed to deviate more than their normal trading activities which are vulnerable to exploits.

See POC below as an example to show the above scenario. Full test file here.

//Pump.HardCap.t.sol
...
 function setUp() public {
        mWell = new MockReserveWell();
        initUser();
        pump = new MultiFlowPump(
            from18(0.5e18),
            from18(0.333333333333333333e18),
            12,
            from18(0.9e18)
        );
    }

    function test_HardCap() public {
        uint256[] memory initReserves = new uint256[](4);
        //initiate for mWell and pump
        initReserves[0] = 100e8; //wBTC
        initReserves[1] = 1600e18; //wETH
        initReserves[2] = 99000e4; //USDC
        initReserves[3] = 98000e4; //USDT
        mWell.update(address(pump), initReserves, new bytes(0));
        increaseTime(12);
        //Reserve update
        uint256[] memory updateReserves = new uint256[](4);
        updateReserves[0] = 160e8; //wBTC
        updateReserves[1] = 1000e18; //wETH
        updateReserves[2] = 96000e4; //USDC
        updateReserves[3] = 101000e4; //USDT
        mWell.update(address(pump), updateReserves, new bytes(0));
        // lastReserves0 reflects initReserves[i]
        uint256[] memory lastReserves0 = pump.readLastReserves(address(mWell));
        increaseTime(12);
        mWell.update(address(pump), updateReserves, new bytes(0));
        // lastReserves1 reflects 1st reserve update
        uint256[] memory lastReserves1 = pump.readLastReserves(address(mWell));
        assertEq(
            ((lastReserves1[0] - lastReserves0[0]) * 1000) / lastReserves0[0],
            500
        ); //wBTC: 50% reserve change versus 60% reserve change in well
        console.log(lastReserves1[0]); //14999999999
        assertEq(
            ((lastReserves0[1] - lastReserves1[1]) * 1000) / lastReserves0[1],
            333
        ); //wETH: 33% reserve change versus 37.5% reserve change in well
        console.log(lastReserves1[1]); //1066666666666666667199
        assertApproxEqAbs(lastReserves1[2], 96000e4, 1); //USDC: reserve change matches well, 3% change decrease
        assertApproxEqAbs(lastReserves1[3], 101000e4, 1); //USDT: reserve change matches well, 3% reserve increase
    }
...

As seen in POC, USDT/USDC reserve changes of more than 3% are allowed, whereas WBTC/WETH reserve changes are restricted. The test is based on 50% for max percentage per block increase and 33% max percentage per block decrease. Although the exact percentage change settings can be different and the actual token reserve changes per block differ by assets, the idea is such universal cap value likely will not fit multiple tokens or trading pairs. The cap can be either set too wide which is then no need to have a cap at all, or be set in a manner that restricts some volatile token assets.

See test results.

Running 1 test for test/pumps/Pump.HardCap.t.sol:PumpHardCapTest
[PASS] test_HardCap() (gas: 489346)
Logs:
  14999999999
  1066666666666666667199

Test result: ok. 1 passed; 0 failed; finished in 18.07ms

Tools Used

Manual. Vscode.

Recommended Mitigation Steps

Different assets have different levels of volatility, and such variations in volatility are typically taken into account in an AMM or oracle. (Think UniswapV3 with different tick spacings, or chainlink with different price reporting intervals).

It’s better to avoid setting up LOG_MAX_INCREASE, LOG_MIN_INCREASE directly in MultiFlowPump.sol.

(1) Instead, in Well.sol allow MAX_INCREASE and MAX_DECREASE to be token specific and included as part of the immutable data created at the time of Well.sol development from Aquifer.sol, such that a token address can be included together with MAX_INCREASE and MAX_DECREASE as immutable constants.

(2) Then in _updatePumps(), pass current token reserves, and pass token MAX_INCREASE and MAX_DECREASE as _pump.data to MultiFlowPump.sol update().

(3) In MultiFlowPump.sol update() , when calculating _capReserve() , use decoded _pump.data to pass MAX_INCREASE, MAX_DECREASE for token specific cap calcuation.

Assessed type

Oracle

No event emitting in MultiFlowPump.sol for off-chain accounting

MultFlowPump.sol is designed to be minimalistic in a way that only necessary data is stored on-chain. So it’s an intended behavior some data can be easily acquired and stored off-chain for customized use cases.

For example, for the TwaReserves feature to be sufficient, some off-chain accounting might be needed to store historic CumulativeReserves values and their timestamps.

However, the current implementation of MultiFlowPump.sol doesn’t provide such easy access for off-chain mechanisms because there are no events that can be emitted.

Recommendation

It’s recommended to properly emit events of change in these calculated reserve values and their timestamps for proper off-chain accounting.

Wasteful gas operation is performed during an update to pumps in the same block

When MultiFlowPump.sol update() is called, blockPassed is calculated based on deltaTimestamp. When the function is called during the same block as the last reserve update, deltaTimestamp will be 0 and blockPassed is 0.

This is the condition where wasteful gas occurs.

In MultiFlowPump.sol update(), lastReserves[i] is designed to be the same value as the previous update with the same timestamp as the last timestamp. cumulativeReserve[i] will be the same as the previous cumulativeReserve[i].

//MultiFlowPump.sol-update()
...
       {
            uint256 deltaTimestamp = _getDeltaTimestamp(
                pumpState.lastTimestamp
            );
            alphaN = ALPHA.powu(deltaTimestamp);
            deltaTimestampBytes = deltaTimestamp.fromUInt();
            // Relies on the assumption that a block can only occur every `BLOCK_TIME` seconds.
            blocksPassed = (deltaTimestamp / BLOCK_TIME).fromUInt();
        }
...
            pumpState.lastReserves[i] = _capReserve(
                pumpState.lastReserves[i],
                (reserves[i] > 0 ? reserves[i] : 1).fromUIntToLog2(),
                blocksPassed
            );
...
slot.storeLastReserves(uint40(block.timestamp), pumpState.lastReserves);
...

//MultiFlowPump.sol-_capReserve()
...
        if (lastReserve.cmp(reserve) == 1) {
            bytes16 minReserve = lastReserve.add(
                blocksPassed.mul(LOG_MAX_DECREASE)
            );
            // if reserve < minimum reserve, set reserve to minimum reserve
            if (minReserve.cmp(reserve) == 1) reserve = minReserve;
        }
...

Even though lastReserve[i], cumulativeReserve[i], and lastTimestamp don’t change, these values will still be calculated inside _capReserve and then written to storage. This is a wasteful calculation and storage operation that can be bypassed to save gas.

Recommendation

Bypass lastReserves[i] cumulativeReserves[i] calculation and re-writing to storage when deltaTimestamp is 0.

Any comments for the judge to contextualize your findings

The main focus of the audit is to:

1. Ensure core external-facing contracts operate as intended as outlined in the docs.
2. Identify potential security issues, especially opportunities for exploits within the smart contracts.
3. Special focus was paid to the protocol goals of achieving 'composability' in AMMs. Evaluate how the current design of pumps and wells might compromise the purposed 'composability', as well as how they might compromise common essential features of popular AMM protocols like Uniswap as a trade-off.

Note that library contracts were excluded from my audit.

Approach taken in evaluating the codebase

1. Manual code walkthrough.
2. Manual assessment of variables to identify any arithmetic-related vulnerabilities.
3. Testing with custom scripts. (Foundry)

Architecture recommendations

Since 'well functions' is the core that enables different flavors of trade, it might be debatable whether a design pattern that completely separates 'well functions' and 'wells' will limit the versatility of AMMs.

An example that might challenge this pattern is UniswapV3, or similar AMMs that provide concentrated liquidity. In UniswapV3, liquidity is nonfungible and the current frameworks of Well.sol are insufficient in supporting nonfungible liquidity. In addition, in 'well functions', the relations between total supply and token reserve do not exist in this scenario.

One recommendation is to explore how an ERC721-based well might achieve a clean separation between well and well functions for protocols like UniswapV3. Otherwise, might need to consider a new group of wells with integrated well functions that deal with nonfungible liquidity which compromises 'composability' on some level.

Codebase quality analysis

1. Documentation: The code is largely well documented with some exceptions in Well.sol, MultiFlowPump.sol, ConstantProduct2.sol where public-facing functions have no or insufficient Natspec comments. There are inconsistent Natspec styles in ConstantProduct2.sol.

2. Tests: There are uncovered areas, although the extent is not assessed. For example, in Pump.Fuzz.t.sol, there is no testing on EMA values -emaReserves[i] when there is testing on the last reserves and cumulative reserves.

3. Gas: Wasteful gas operation is observed. The report is submitted separately.

Centralization risks

Very low risk of centralization

Mechanism review

One concern on the assumptions in Well.sol is the acceptance of uneven liquidity adding or removing. As an example of a good well implementation, Well.sol allows various uneven and single-sided liquidity modifications by users with no constraints.

Although this is a design choice, it allows token reserve ratios in a market to change outside of normal trading activities, which can be considered a vulnerable feature for an AMM.

Note that as an alternative, single-sided liquidity modification can be achieved by including token swapping, in which case it can be an add-on feature without impacting ratios of token reserves.

Systemic risks

In my opinion, the pump design is crucial in ensuring overall system health.

1. The current MultiFlowPump.sol implementation has some weaknesses in handling multiple token prices.

2. In addition, there seems to be no implementation in place to allow multiple wells to share a pump. For example, the doc says a pump can be shared by multiple wells if it stores a mapping of well addresses. Such implementation is not found.

3. Because pumps are intended to be designed in a minimalistic way, further tests and example implementation of how pumps' core functionality such as EMA, SMA, and TWA values can be extended on-chain and off-chain need to be looked into.

Time spent:

11 hours