Basin - sces60107's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/main/src/Aquifer.sol#L34

Vulnerability details

Impact

A benign user can call Aquifer.boreWell with salt to deploy a new Well with create2. However the implementation of Aquifer.boreWell is vulnerable to front-running attacks. A malicious user can front-run the transaction with malicious initFunctionCall.

Proof of Concept

Aquifer.boreWell uses create2 to deploy a new Well if salt > 0. And then it performs well.call(initFunctionCall) if initFunctionCall.length > 0. https://github.com/code-423n4/2023-07-basin/blob/main/src/Aquifer.sol#L34

    function boreWell(
        address implementation,
        bytes calldata immutableData,
        bytes calldata initFunctionCall,
        bytes32 salt
    ) external nonReentrant returns (address well) {
        if (immutableData.length > 0) {
            if (salt != bytes32(0)) {
                well = implementation.cloneDeterministic(immutableData, salt);
            } else {
                well = implementation.clone(immutableData);
            }
        } else {
            if (salt != bytes32(0)) {
                well = implementation.cloneDeterministic(salt);
            } else {
                well = implementation.clone();
            }
        }

        if (initFunctionCall.length > 0) {
            (bool success, bytes memory returnData) = well.call(initFunctionCall);
            if (!success) {
                // Next 5 lines are based on https://ethereum.stackexchange.com/a/83577
                if (returnData.length < 68) revert InitFailed("");
                assembly {
                    returnData := add(returnData, 0x04)
                }
                revert InitFailed(abi.decode(returnData, (string)));
            }
        }

        …
    }

Since the salt is unrelated to the initFunctionCall. A malicious user can front-run the transaction with malicious initFunctionCall. Since the well implementation is user-defined, the impact of this attack could be significant.

Tools Used

Manual Review

Recommended Mitigation Steps

There are two methods to mitigate this issue: Add msg.sender to the salt

    function boreWell(
        address implementation,
        bytes calldata immutableData,
        bytes calldata initFunctionCall,
        bytes32 salt
    ) external nonReentrant returns (address well) {
        if (immutableData.length > 0) {
            if (salt != bytes32(0)) {
-               well = implementation.cloneDeterministic(immutableData, salt);
+               well = implementation.cloneDeterministic(immutableData, keccak256(abi.encodePacked(salt, msg.sender)));
            } else {
                well = implementation.clone(immutableData);
            }

Or add initFunctionCall to the salt

    function boreWell(
        address implementation,
        bytes calldata immutableData,
        bytes calldata initFunctionCall,
        bytes32 salt
    ) external nonReentrant returns (address well) {
        if (immutableData.length > 0) {
            if (salt != bytes32(0)) {
-               well = implementation.cloneDeterministic(immutableData, salt);
+               well = implementation.cloneDeterministic(immutableData, keccak256(abi.encodePacked(salt, initFunctionCall)));
            } else {
                well = implementation.clone(immutableData);
            }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/main/src/pumps/MultiFlowPump.sol#L27 https://github.com/code-423n4/2023-07-basin/blob/main/src/pumps/MultiFlowPump.sol#L119 https://github.com/code-423n4/2023-07-basin/blob/main/src/Well.sol#L645

Vulnerability details

Impact

The comment in MultiFlowPump.sol says: https://github.com/code-423n4/2023-07-basin/blob/main/src/pumps/MultiFlowPump.sol#L27

 * Note: If an `update` call is made with a reserve of 0, the Geometric mean oracles will be set to 0.
 * Each Well is responsible for ensuring that an `update` call cannot be made with a reserve of 0.
 */

However, the current implementation of Well doesn’t ensure that an update call cannot be made with a reserve of 0.

Proof of Concept

The update call is made in Well._updatePumps. It is obvious that it doesn’t ensure that the reserves are not zero. https://github.com/code-423n4/2023-07-basin/blob/main/src/Well.sol#L645

    function _updatePumps(uint256 _numberOfTokens) internal returns (uint256[] memory reserves) {
        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.
            }
        } else {
            Call[] memory _pumps = pumps();
            for (uint256 i; i < _pumps.length; ++i) {
                // Don't revert if the update call fails.
                try IPump(_pumps[i].target).update(reserves, _pumps[i].data) {}
                catch {
                    // ignore reversion. If an external shutoff mechanism is added to a Pump, it could be called here.
                }
            }
        }
    }

And MultiFlowPump uses a minimum of 1 for reserve if a reserve is 0. It could lead to the wrong result of other view functions. https://github.com/code-423n4/2023-07-basin/blob/main/src/pumps/MultiFlowPump.sol#L119

    function update(uint256[] calldata reserves, bytes calldata) external {
        …

        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
            );
            …
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Well._updatePumps should ensure that an update call cannot be made with a reserve of 0

Assessed type

Other