Basin - Trust's results

Lines of code

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

Vulnerability details

Description

Please refer to the issue titled Implementation of Well shift() function allows attackers to completely manipulate the oracles for relevant introduction and context.

The safety of the TWAP relies on calling the observation function (update()) with the current reserve, before any reserve changes take place. Updating happens in the _updatePumps call:

reserves = _getReserves(_numberOfTokens);
if (numberOfPumps() == 0) {
    return reserves;
}
// gas optimization: avoid looping if there is only one pump
if (numberOfPumps() == 1) {
    Call memory _pump = firstPump();
    // Don't revert if the update call fails.
    try IPump(_pump.target).update(reserves, _pump.data) {}
    catch {
        // ignore reversion. If an external shutoff mechanism is added to a Pump, it could be called here.
    }

There is a bug in the sync() function, whereby the reserves are directly updated from the contract balance, but updatePumps() is not called beforehand.

 /**
 * @dev Sync the reserves of the Well with its current balance of underlying tokens.
 */
function sync() external nonReentrant {
    IERC20[] memory _tokens = tokens();
    uint256[] memory reserves = new uint256[](_tokens.length);
    for (uint256 i; i < _tokens.length; ++i) {
        reserves[i] = _tokens[i].balanceOf(address(this));
    }
    _setReserves(_tokens, reserves);
    emit Sync(reserves);
}

An attacker can abuse this issue by donating ERC20 tokens to one side of the token pair in the Well, calling sync() to update the reserves without updating pumps, and then doing a negligible swap. At the first swapFrom() call, the pumps will be updated. It will appear as if the entire duration until the sync() call was with the wrong ratio. Both the EMA oracle and the Cumulative oracle will be skewed.

Impact

The TWAP oracle can be arbitrarily manipulated by an attacker.

POC

The TWAP can always be manipulated. In order for it to be profitable, the gain from the exploit of a platform dependent on the TWAP needs to be higher than the donation to the Well contract used to skew the ratio. An example is provided:

Assume a Well holding 100 ETH : 100000 USDC, Fair ratio is 1:1000. Last trade happens at t = T Time is now t = T + 1000 sec. Cumulative reserves in oracle contract are [X, Y]. To remind, they are calculated as

pumpState.cumulativeReserves[i] =    pumpState.cumulativeReserves[i].add(pumpState.lastReserves[i].mul(deltaTimestampBytes));

1. Attacker identifies a Lending platform which uses the TWAP to determine ETH/USD value. ETH collat ratio is 90%
2. User calls exploit contract
   • take a flashloan of 1000 ETH
   • transfer 100 ETH to the Well
   • call sync() , new reserves are 200 : 100,000.
   • call swapFrom(), to swap 1 wei of ETH. Pump update is triggered, with new reserve ratio:
   • Cumulative = `[X,Y] + [200 * 1000, 100000 * 1000]
   • Victim queries TWAP price, which calculates:

   twaReserves = ([X + 200*1000, Y + 10000*1000]-[X,Y]) / 1000
   twaReserves = [200, 10000]
   

   • Assumed ETH price is $500 USD
   • exploit contract converts 900 ETH to 900,000 USDC
   • exploit contract deposits 900,000 USDC as collateral
   • exploit contract borrows 900,000 / $500 * 90% (max loan ratio) = 1620 ETH
   • exploit contract repays 1000 ETH flashloan with loaned ETH
   • attacker exits with 620ETH of profit, minus swap fees

Tools Used

Manual audit

Recommended Mitigation Steps

In the sync() function, call the _updatePumps() function, even though the current reserve count is not necessary.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Description

The TWAP mechanism relies on measurements sent to the oracle at various points in time. Before reserve counts change, the TWAP is sent the last reserve counts, which are multiplied by the time passed and added to the accumulator. In MultiFlowPump, it happens here:

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
    );
    pumpState.emaReserves[i] =
        pumpState.lastReserves[i].mul((ABDKMathQuad.ONE.sub(alphaN))).add(pumpState.emaReserves[i].mul(alphaN));
    pumpState.cumulativeReserves[i] =
       // @audit - TWAP update
pumpState.cumulativeReserves[i].add(pumpState.lastReserves[i].mul(deltaTimestampBytes));
}

lastReserves[i] is capped and updated with the parameter reserves[i].price. Then it is used to determine cumulativeReserves[i]. This behavior is correct, as discussed in the Uniswap v3 book.

The safety of the TWAP relies on calling the observation function (update()) with the current reserve, before any reserve changes take place. Updating happens in the _updatePumps call:

reserves = _getReserves(_numberOfTokens);
if (numberOfPumps() == 0) {
    return reserves;
}
// gas optimization: avoid looping if there is only one pump
if (numberOfPumps() == 1) {
    Call memory _pump = firstPump();
    // Don't revert if the update call fails.
    try IPump(_pump.target).update(reserves, _pump.data) {}
    catch {
        // ignore reversion. If an external shutoff mechanism is added to a Pump, it could be called here.
    }

It is called in the following functions which change reserve counts:

1. _swapFrom()
2. swapTo()
3. _addLiquidity()
4. removeLiquidity()
5. removeLiquidityOneToken()
6. removeLiquidityImbalanced()

However, it is not called in the shift() function, which is very similar to swapFrom() and updates reserves counts.

 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);
     (uint256 i, uint256 j) = _getIJ(_tokens, fromToken, toToken);
	 ...
	 
function shift(
    IERC20 tokenOut,
    uint256 minAmountOut,
    address recipient
) external nonReentrant returns (uint256 amountOut) {
    IERC20[] memory _tokens = tokens();
    uint256[] memory reserves = new uint256[](_tokens.length);	 
	...

At the end of the function, it will update the reserves from the current balances:

if (amountOut >= minAmountOut) {
    tokenOut.safeTransfer(recipient, amountOut);
    reserves[j] -= amountOut;
    _setReserves(_tokens, reserves);
    emit Shift(reserves, tokenOut, amountOut, recipient);
} else {
    revert SlippageOut(amountOut, minAmountOut);
}

This means an attacker can easily manipulate the TWAP price. They will transfer tokens to the contract and use shift() to change the reserve count to an extreme ratio, then swap back the tokens using a normal swapFrom() call, which will set update the oracle with the post-shift reserve counts. It will appear as if the entire duration until the shift was with the extreme ratio. Both the EMA oracle and the Cumulative oracle will be skewed.

Impact

The TWAP oracle can be arbitrarily manipulated by an attacker.

POC

Assume a Well holding 100 ETH : 100000 USDC, Fair ratio is 1:1000. Last trade happens at t = T Time is now t = T + 1000 sec. Cumulative reserves in oracle contract are [X, Y]. To remind, they are calculated as

pumpState.cumulativeReserves[i] =    pumpState.cumulativeReserves[i].add(pumpState.lastReserves[i].mul(deltaTimestampBytes));

1. User calls exploit contract
   • take a flashloan of 10,000 ETH
   • transfer all ETH to the Well
   • call shift() , new reserves are 10100 : 990. Receive 100000-990 USDC to wallet
   • call swapFrom(), passing received USDC from shift(). Pool returns to 100 : 100,000, user receives 10,000 ETH back
   • user repays flashloan
2. During swapFrom(), cumulative reserves were updated:
   • Cumulative = `[X,Y] + [10100 * 1000, 990 * 1000]
3. The TWAP oracle has been poisoned.

For example, a victim now queries the TWAP price in the last 1000 sec, using readTwaReserves(Well, [x,y], T): It will calculate:

twaReserves = ([X + 10100*1000, Y + 990*1000]-[X,Y]) / 1000
twaReserves = [10100, 990]

Calculation shows the TWAP price for the past 1000 seconds is 1 ETH = 0.098 USDC. Attacker can now exploit any user of the TWAP through uncollateralized loans, swaps at wrong ratio, or any other target-specific way.

Tools Used

Manual audit

Recommended Mitigation Steps

In the shift() function, call the _updatePumps() function, even though the current reserve count is not necessary.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibBytes.sol#L23

Vulnerability details

Description

The LibBytes library is used to read and store uint128 types compactly for Well functions. The function getBytes32FromBytes() will fetch a specific index as bytes32.

/**
 * @dev Read the `i`th 32-byte chunk from `data`.
 */
function getBytes32FromBytes(bytes memory data, uint256 i) internal pure returns (bytes32 _bytes) {
    uint256 index = i * 32;
    if (index > data.length) {
        _bytes = ZERO_BYTES;
    } else {
        assembly {
            _bytes := mload(add(add(data, index), 32))
        }
    }
}

If the index is out of bounds in the data structure, it returns ZERO_BYTES = bytes32(0). The issue is that the OOB check is incorrect. If index=data.length, the request is also OOB. For example: data.length=0 -> array is empty, data[0] is undefined. data.length=32 -> array has one bytes32, data[32] is undefined.

In other words, fetching the last element in the array will return whatever is stored in memory after the bytes structure.

Impact

Users of getBytes32FromBytes will receive arbitrary incorrect data. If used to fetch reserves like readUint128, this could easily cause severe damage, like incorrect pricing, or wrong logic that leads to loss of user funds.

POC

The damage is easily demonstrated using the example below:

pragma solidity 0.8.17;

contract Demo {
    event Here();
    bytes32 private constant ZERO_BYTES = bytes32(0);


    function corruption_POC(bytes memory data1, bytes memory data2) external returns (bytes32 _bytes) {
        _bytes = getBytes32FromBytes(data1, 1);
    }

    function getBytes32FromBytes(bytes memory data, uint256 i) internal returns (bytes32 _bytes) {
        uint256 index = i * 32;
        if (index > data.length) {
            _bytes = ZERO_BYTES;
        } else {
            emit Here();
            assembly {
                _bytes := mload(add(add(data, index), 32))
            }
        }
    }
}

Calling corruption_POC with the following parameters:

{
	"bytes data1": "0x2222222222222222222222222222222222222222222222222222222222222222",
	"bytes data2": "0x3333333333333333333333333333333333333333333333333333333333333333"
}

The output bytes32 is:

{
	"0": "bytes32: _bytes 0x0000000000000000000000000000000000000000000000000000000000000020"
}

The 0x20 value is in fact the size of the data2 bytes that resides in memory from the call to getBytes32FromBytes.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the logic to:

if (index >= data.length) {
    _bytes = ZERO_BYTES;
} else {
    assembly {
        _bytes := mload(add(add(data, index), 32))
    }
}

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibBytes16.sol#L45 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibLastReserveBytes.sol#L58

Vulnerability details

Description

The MultiFlowPump contract stores reserve counts on every update, using the libraries LibBytes16 and LibLastReserveBytes. Those libs pack bytes16 values efficiently with the storeBytes16() and storeLastReserves functions. In case of an odd number of items, the last storage slot will be half full. Care must be taken to not step over the previous value in that slot. This is done correctly in LibBytes:

if (reserves.length & 1 == 1) {
    require(reserves[reserves.length - 1] <= type(uint128).max, "ByteStorage: too large");
    iByte = maxI * 64;
    assembly {
        sstore(
            // @audit - overwrite SLOT+MAXI*32
            add(slot, mul(maxI, 32)),
                                                       // @audit - read SLOT+MAXI*32 (2nd entry)
            add(mload(add(reserves, add(iByte, 32))), shl(128, shr(128, sload(add(slot, mul(maxI, 32))))))
        )
    }
}

As can be seen, it overwrites the slot with the previous 128 bits in the upper half of the slot, only setting the lower 128 bytes.

However, the wrong slot is read in the other two libraries. For example, in storeLastReserves():

if (reserves.length & 1 == 1) {
    iByte = maxI * 64;
    assembly {
        sstore(
            // @audit - overwrite SLOT+MAXI*32
            add(slot, mul(maxI, 32)),
                                                      // @audit - read SLOT+MAXI (2nd entry)??
            add(mload(add(reserves, add(iByte, 32))), shr(128, shl(128, sload(add(slot, maxI)))))
        )
    }
}

The error is not multiplying maxI before adding it to slot. This means that the reserves count encoded in lower 16 bytes in add(slot, mul(maxI, 32)) will have the value of a reserve in a much lower index. Slots are used in 32 byte increments, i.e. S, S+32, S+64... When maxI==0, the intended slot and the actual slot overlap. When maxI is 1..31, the read slot happens to be zero (unused), so the first actual corruption occurs on maxI==32. By substitution, we get: SLOT[32*32] = correct reserve | SLOT[32] In other words, the 4rd reserve (stored in lower 128 bits of SLOT[32]) will be written to the 64th reserve.

The Basin pump is intended to support an arbitrary number of reserves safely, therefore the described storage corruption impact is in scope.

Impact

Reserve amounts in the pump will be corrupted, resulting in wrong oracle values.

POC

1. A reserve update is triggered on the pump when some Well action occurs.
2. Suppose reserves are array [0,1,2,...,63,64]
3. Reserve count is odd, so affected code block is reached
4. SLOT[32*32] = UPPER: 64 | LOWER: SLOT[32] = 64 | 3

Tools Used

Manual audit

Recommended Mitigation Steps

Change the sload() operation in both affected functions to sload(add(slot, mul(maxI, 32)

Assessed type

en/de-code

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibBytes16.sol#L45

Vulnerability details

Description

It is advised to first read finding: Due to slot confusion, reserve amounts in the pump will be corrupted, resulting in wrong oracle values, which provides all the contextual information for this separate bug.

We've discussed how a wrong sload() source slot leads to corruption of the reserves. In LibBytes16, another confusion occurs. Recall the correct storage overwriting done in LibBytes:

assembly {
    sstore(
        // @audit - overwrite SLOT+MAXI*32
        add(slot, mul(maxI, 32)),
                                                   // @audit - read SLOT+MAXI*32 (2nd entry)
        add(mload(add(reserves, add(iByte, 32))), shl(128, shr(128, sload(add(slot, mul(maxI, 32))))))
    )
}

Importantly, it clears the lower 128 bytes of the source slot and replaces the upper 128 bytes of the dest slot using the upper 128 bytes of the source slot: shl(128,shr(128,SOURCE))

In storeBytes16(), the shl() operation has been discarded. This means the code will use the upper 128 bytes of SOURCE to overwrite the lower 128 bytes in DEST.

if (reserves.length & 1 == 1) {
    iByte = maxI * 64;
    assembly {
        sstore(
        // @audit - overwrite SLOT+MAXI*32
            add(slot, mul(maxI, 32)),
                                                    // @audit - overwrite lower 16 bytes with upper 16 bytes?
            add(mload(add(reserves, add(iByte, 32))), shr(128, sload(add(slot, maxI))))
        )
    }
}

In other words, regardless of the SLOT being read, instead of keeping the lower 128 bits as is, it stores whatever happens to be in the upper 128 bits. Note this is a completely different error from the slot confusion, which happens to be in the same line of code.

Impact

Reserve amounts in the pump will be corrupted, resulting in wrong oracle values

POC

Assume slot confusion bug has been corrected for clarity.

1. A reserve update is triggered on the pump when some Well action occurs.
2. Suppose reserves are array [0,1,2,...,63,64]
3. Suppose previous reserves are array [P0,P1,...,P64]
4. Reserve count is odd, so affected code block is reached
5. SLOT[32*32] = UPPER: 64 | LOWER: UPPER(SLOT[32*32]) = 64 | P64

Tools Used

Manual audit

Recommended Mitigation Steps

Replace the affected line with the calculation below: shr(128, shl(128, SLOT)) This will use the lower 128 bytes and clear the upper 128 bytes,as intended.

Assessed type

en/de-code

Lines of code

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

Vulnerability details

Description

The Well allows users to permissionless swap assets or add and remove liquidity. Users specify the intended slippage in swapFrom, in minAmountOut.

The ConstantProduct2 implementation ensures Kend - Kstart >= 0, where K = Reserve1 * Reserve2, and the delta should only be due to tiny precision errors.

Furthermore, the Well does not impose any fees to its users. This means that all conditions hold for a successful DOS of any swap transactions.

1. Token cost of sandwiching swaps is zero (no fees) - only gas cost
2. Price updates are instantenous through the billion dollar formula.
3. Swap transactions along with the max slippage can be viewed in the mempool

Note that such DOS attacks have serious adverse effects both on the protocol and the users. Protocol will use users due to disfunctional interactions. On the other side, users may opt to increment the max slippage in order for the TX to go through, which can be directly abused by the same MEV bots that could be performing the DOS.

Impact

All swaps can be reverted at very little cost.

POC

1. Evil bot sees swap TX, slippage=S
2. Bot submits a flashbot bundle, with the following TXs
   1. Swap TX in the same direction as victim, to bump slippage above S
   2. Victim TX, which will revert
   3. Swap TX in the opposite direction and velocity to TX (1). Because of the constant product formula, all tokens will be restored to the attacker.

Tools Used

Manual audit

Recommended Mitigation Steps

Fees solve the problem described by making it too costly for attackers to DOS swaps. If DOS does takes place, liquidity providers are profiting a high APY to offset the inconvenience caused, and attract greater liquidity.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/pumps/MultiFlowPump.sol#L39

Vulnerability details

Description

Pumps receive the chain BLOCK_TIME in the constructor. In every update, it is used to calculate the blocksPassed variable, which determines what is the maximum change in price (done in _capReserve()).

The issue is that BLOCK_TIME is an immutable variable in the pump, which is immutable in the Well, meaning it is basically set in stone and can only be changed through a Well redeploy and liquidity migration (very long cycle). However, BLOCK_TIME actually changes every now and then, especially in L2s.For example, the recent Bedrock upgrade in Optimism completely changed the block time generation. It is very clear this will happen many times over the course of Basin's lifetime.

When a wrong BLOCK_TIME is used, the _capReserve() function will either limit price changes too strictly, or too permissively. In the too strict case, this would cause larger and large deviations between the oracle pricing and the real market prices, leading to large arb opportunities. In the too permissive case, the function will not cap changes like it is meant too, making the oracle more manipulatable than the economic model used when deploying the pump.

Impact

Treating of BLOCK_TIME as permanent will cause serious economic flaws in the oracle when block times change.

Tools Used

Manual audit

Recommended Mitigation Steps

The BLOCK_TIME should be changeable, given a long enough freeze period where LPs can withdraw their tokens if they are unsatisfied with the change.

Assessed type

Timing

Lines of code

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

Vulnerability details

Description

In Well, the shift() function is used to transfer tokens directly to the next Well instead of the sender. This allows for a cheap sequence of swaps. The documentation states the exact call flow:

* 2. Using a router with {shift}:
*  WETH.transfer(sender=0xUSER, recipient=Well1)                        [1]
*  Call the router, which performs:
*      Well1.shift(tokenOut=DAI, recipient=Well2)
*          DAI.transfer(sender=Well1, recipient=Well2)                  [2]
*      Well2.shift(tokenOut=USDC, recipient=0xUSER)
*          USDC.transfer(sender=Well2, recipient=0xUSER)                [3]

Notice that there are two separate actions, first (1) transfering the token (WETH) to the Well, and then (2) calling the router's function, which will shift the tokens to a second Well, and finally to the user's balance.

The issue is that between action (1) and (2) a malicious user can use the victim tokens from (1) as their own amountIn. They can call one of the Well functions, like shift(), to perform the swap and specify the attacker as recipient.

From all the documentation provided and the codebase, the user is led to interact unsafely with the Well's shift() call.

Impact

Users following the documented action flow can lose their entire input amount.

POC

1. User A sends WETH tokens to Well1
2. User A calls the router's multi-Well shift function
3. User B calls shift(tokenOut, ATTACKER_ADDRESS) on Well directly. Since (1) and (2) are different user initiated TXs, there is definitely vulnerable time to abuse the extra token balance in Well1.

Tools Used

Manual audit

Recommended Mitigation Steps

Users should not interact with the Well directly. They should call a router function which will do all the swapping activity end-to-end.

Assessed type

MEV

General

I've spent 2-3 days on this audit and covered the code in scope. Having been audited twice by private firms, I expected there to be very little in terms of low-hanging fruits.

Approach

Most of the focus was on deeply understanding the assembly optimizations and particular LP mechanics, informally proving their correctness and so on. Differential analysis between Basin and Uniswap was helpful for this task.

Architecture recommendations

The platform supports a very generic well implementation, and as the team understands, this leads to a broad variety of malicious deployed well risks. It should be a high priority task for the team to find good ways of representing all Well trust assumptions to its users, and expose that through a smart contract UI or an open source front-end library.

Qualitative analysis

Code quality and documentation is very mature. The test suite is pretty comprehensive and fuzz tests are a great way to complement the static tests. My suggestion is to add integration tests to verify behavior of the system as a whole, rather than all its specific sub-components.

Centralization risks

None. The architecture is fully permissionless.

Systemic risks

MEV and TWAP manipulation are the main systemic risks. Interacting with Wells registered in the Aquifer could possibly be risky, depending on the well's configuration.

The immutability of the Pump and Well co-efficients, such as LOG_MAX_INCREASE and ALPHA, present a systemic risk as time progresses and the optimal values start diverging.

As the entire platform is unupgradeable, migration in the event of a security hole or a black swan event will be challenging. The team should prepare multiple recovery scenarios and set up adequate action channels to prepare for such eventualities.

Time spent:

25 hours