BitFluxFi - Stable AMM - CoreDAO

The removeBaseLiquidityOneToken function in the Router contract uses the approve method to set allowances. Unlike other functions such as removeLiquidity, which use safeIncreaseAllowance, this inconsistency can introduce a potential approval race condition risk. The approve function can be exploited if a spender front-runs the transaction and uses the current allowance before it is updated. This could lead to unintended token transfers, posing a security risk if not handled properly.

Code Location

Use of the approve method to set allowances:

    function removeBaseLiquidityOneToken(
        ISwap pool,
        ISwap basePool,
        uint256 _token_amount,
        uint8 i,
        uint256 _min_amount,
        uint256 deadline
    ) external returns (uint256) {
        IERC20 token = pool.getLpToken();
        IERC20 baseToken = basePool.getLpToken();
        uint8 baseTokenIndex = pool.getTokenIndex(address(baseToken));
        token.transferFrom(msg.sender, address(this), _token_amount);
        token.approve(address(pool), _token_amount);
        pool.removeLiquidityOneToken(_token_amount, baseTokenIndex, 0, deadline);
        uint256 _base_amount = baseToken.balanceOf(address(this));
        baseToken.approve(address(basePool), _base_amount);
        basePool.removeLiquidityOneToken(_base_amount, i, _min_amount, deadline);
        IERC20 coin = basePool.getToken(i);
        uint256 coin_amount = coin.balanceOf(address(this));
        coin.safeTransfer(msg.sender, coin_amount);
        return coin_amount;
    }

Replace the use of approve with safeIncreaseAllowance to align with best practices and ensure consistent allowance handling.

RISK ACCEPTED: The BitFlux team accepted this risk of this finding and stated the following:

We acknowledge that the approve method can introduce a race condition if a spender front-runs the transaction and uses the current allowance before it is updated. However, in our contract, this risk is mitigated by the specific context in which approve is used. The approve function is only called within tightly controlled internal logic, where the sequence of operations ensures that no external actor can exploit the allowance before it is consumed. Additionally, we use approve only with tokens that strictly follow the ERC-20 standard, which further reduces the likelihood of an issue. 

The addLiquidity function in the Router contract has a potential issue where meta_amounts[i] is overwritten by base_lp_received when coin matches base_lp. This leads to two scenarios:

1. If the initial meta_amounts[i] is lower than base_lp_received, the user is compelled to use the higher base_lp_received value, potentially depositing more tokens than intended.

   
   

2. If the initial meta_amounts[i] is greater than base_lp_received, the user can only deposit up to base_lp_received, restricting the ability to contribute the desired amount.

This behavior reduces flexibility and can result in unexpected outcomes for users who intend to provide a specific amount of liquidity.

Code Location

meta_amounts[i] is overwritten by base_lp_received when coin matches base_lp:

    function addLiquidity(
        ISwap pool,
        ISwap basePool,
        uint256[] memory meta_amounts,
        uint256[] memory base_amounts,
        uint256 minToMint,
        uint256 deadline
    ) external returns (uint256) {
        IERC20 token = IERC20(pool.getLpToken());
        IERC20 base_lp = IERC20(basePool.getLpToken());
        require(base_amounts.length == basePool.getNumberOfTokens(), "invalidBaseAmountsLength");
        require(meta_amounts.length == pool.getNumberOfTokens(), "invalidMetaAmountsLength");
        bool deposit_base = false;
        for (uint8 i = 0; i < base_amounts.length; i++) {
            uint256 amount = base_amounts[i];
            if (amount > 0) {
                deposit_base = true;
                IERC20 coin = basePool.getToken(i);
                uint256 transferred = transferIn(coin, msg.sender, amount);
                coin.safeIncreaseAllowance(address(basePool), transferred);
                base_amounts[i] = transferred;
            }
        }

        uint256 base_lp_received;
        if (deposit_base) {
            base_lp_received = basePool.addLiquidity(base_amounts, 0, deadline);
        }

        for (uint8 i = 0; i < meta_amounts.length; i++) {
            IERC20 coin = pool.getToken(i);

            uint256 transferred;
            if (address(coin) == address(base_lp)) {
                transferred = base_lp_received;
            } else if (meta_amounts[i] > 0) {
                transferred = transferIn(coin, msg.sender, meta_amounts[i]);
            }

            meta_amounts[i] = transferred;
            if (transferred > 0) {
                coin.safeIncreaseAllowance(address(pool), transferred);
            }
        }

        uint256 base_lp_prior = base_lp.balanceOf(address(this));
        pool.addLiquidity(meta_amounts, minToMint, deadline);
        if (deposit_base) {
            require((base_lp.balanceOf(address(this)) + base_lp_received) == base_lp_prior, "invalidBasePool");
        }

        uint256 lpAmount = token.balanceOf(address(this));
        token.transfer(msg.sender, lpAmount);
        return lpAmount;
    }

Adjust the logic to ensure that the value of meta_amounts[i] aligns with user input while maintaining proper handling of base_lp tokens. This change should prevent unnecessary overwriting and allow users to contribute liquidity according to their preferences.

RISK ACCEPTED: The BitFlux team accepted this risk of this finding and stated the following:

We acknowledge that there is potential for user-specified values in meta_amounts[i] to be overwritten by base_lp_received. However, this behavior is intentional and designed to ensure that liquidity contributions are handled efficiently. In cases where users provide liquidity with both base LP tokens and meta tokens, it is necessary to adjust meta_amounts[i] to reflect the actual amount received from the base pool. This is to make sure accurate accounting and prevents discrepancies between user expectations and actual liquidity added. 

Context

The stable-amm is an implementation of Curve's stable swap, and the Halborn team verified the proper implementation by inspecting the differences with the SwapTemplateBase.vy contract from the b0bbf77 commit.

Inconsistencies were identified in:

• calculateTokenAmount from SwapUtils.sol

• getAdminbalance from Swap.sol

• stopRampA from AmplificationUtils.sol

• killMe feature

Calculate token amount

It was found that the calculateTokenAmount of the SwapUtils.sol library returns a positive amount when the total supply of the liquidity pool is zero, while the original Curve implementation simply returns a zero amount. The affected function is only for view purposes so it does not affect the pool state, but could cause unplanned behaviour in external contracts.

• The Curve implementation in SwapTemplateBase.vy:

def calc_token_amount(_amounts: uint256[N_COINS], _is_deposit: bool) -> uint256:
    """
    @notice Calculate addition or reduction in token supply from a deposit or withdrawal
    @dev This calculation accounts for slippage, but not fees.
         Needed to prevent front-running, not for precise calculations!
    @param _amounts Amount of each coin being deposited
    @param _is_deposit set True for deposits, False for withdrawals
    @return Expected amount of LP tokens received
    """
    amp: uint256 = self._A()
    balances: uint256[N_COINS] = self.balances
    D0: uint256 = self._get_D_mem(balances, amp)
    for i in range(N_COINS):
        if _is_deposit:
            balances[i] += _amounts[i]
        else:
            balances[i] -= _amounts[i]
    D1: uint256 = self._get_D_mem(balances, amp)
    token_amount: uint256 = CurveToken(self.lp_token).totalSupply()
    diff: uint256 = 0
    if _is_deposit:
        diff = D1 - D0
    else:
        diff = D0 - D1
    return diff * token_amount / D0

• The Bitflux implementation in SwapUtils.sol the new stable pool invariant d1 is returned:

function calculateTokenAmount(
    Swap storage self,
    uint256[] calldata amounts,
    bool deposit
) external view returns (uint256) {
    uint256 a = _getAPrecise(self);
    uint256[] memory balances = self.balances;
    uint256[] memory multipliers = self.tokenPrecisionMultipliers;

    uint256 d0 = getD(_xp(balances, multipliers), a);
    for (uint256 i = 0; i < balances.length; i++) {
        if (deposit) {
            balances[i] = balances[i].add(amounts[i]);
        } else {
            balances[i] = balances[i].sub(
                amounts[i],
                "Cannot withdraw more than available"
            );
        }
    }
    
    uint256 d1 = getD(_xp(balances, multipliers), a);
    uint256 totalSupply = self.lpToken.totalSupply();

    if (totalSupply == 0) {
        return d1; // first depositor take it all
    }

    if (deposit) {
        return d1.sub(d0).mul(totalSupply).div(d0);
    } else {
        return d0.sub(d1).mul(totalSupply).div(d0);
    }
}

Get admin balance

It was found that the getAdminbalance from the Swap.sol contract was not following the same logic than the admin_balances counterpart in the Curve implementation.

• The Curve implementation in SwapTemplateBase.vy:

def admin_balances(i: uint256) -> uint256:
    return ERC20(self.coins[i]).balanceOf(self) - self.balances[i]

• The Bitflux implementation in Swap.sol:

function getAdminBalances() external view returns (uint256[] memory adminBalances) {
    uint256 length = swapStorage.pooledTokens.length;
    adminBalances = new uint256[](length);
    for (uint256 i = 0; i < length; i++) {
        adminBalances[i] = swapStorage.getAdminBalance(i);
    }
}

Stop ramp A

It was found that the stopRampA of the AmplificationUtils.sol library included an additional check, in comparison to the original implementation. The stopRampA function stops the transition of the amplifier factor of the stable pool, during its transition. That additional check only makes sure that the function cannot be called outside a transition, which has essentially no impact.

• The Curve implementation in SwapTemplateBase.vy:

def stop_ramp_A():
    assert msg.sender == self.owner  # dev: only owner

    current_A: uint256 = self._A()
    self.initial_A = current_A
    self.future_A = current_A
    self.initial_A_time = block.timestamp
    self.future_A_time = block.timestamp
    # now (block.timestamp < t1) is always False, so we return saved A

    log StopRampA(current_A, block.timestamp)

• The Bitflux implementation in AmplificationUtils.sol (note that the onlyOwner modifier is present upstream):

function stopRampA(SwapUtils.Swap storage self) external {
    require(self.futureATime > block.timestamp, "Ramp is already stopped");

    uint256 currentA = _getAPrecise(self);
    self.initialA = currentA;
    self.futureA = currentA;
    self.initialATime = block.timestamp;
    self.futureATime = block.timestamp;

    emit StopRampA(currentA, block.timestamp);
}

Kill feature

The original Curve contract has a killMe feature that allows an owner to shut down a contract, also able to revive it later. It was found that the current implementation does not support this feature. An external contract may assume this feature to be implemented an malfunction as it is not.

• The kill_me definition in SwapTemplateBase.vy:

is_killed: bool
kill_deadline: uint256
KILL_DEADLINE_DT: constant(uint256) = 2 * 30 * 86400

• The kill_me toggles in SwapTemplateBase.vy:

@external
def kill_me():
    assert msg.sender == self.owner  # dev: only owner
    assert self.kill_deadline > block.timestamp  # dev: deadline has passed
    self.is_killed = True


@external
def unkill_me():
    assert msg.sender == self.owner  # dev: only owner
    self.is_killed = False

• Example usage of the kill_me feature in the add_liquidity function of SwapTemplateBase.vy:

@external
@nonreentrant('lock')
def add_liquidity(_amounts: uint256[N_COINS], _min_mint_amount: uint256) -> uint256:
    """
    @notice Deposit coins into the pool
    @param _amounts List of amounts of coins to deposit
    @param _min_mint_amount Minimum amount of LP tokens to mint from the deposit
    @return Amount of LP tokens received by depositing
    """
    assert not self.is_killed  # dev: is killed

    amp: uint256 = self._A()
    old_balances: uint256[N_COINS] = self.balances

It is recommended to review the differences between the origin implementation and the current one, and make sure that all deviations are design choices.

NOT APPLICABLE: The BitFlux team highlighted that the issue is not applicable and mentioned the following:

We recognize that our implementation differs from Curve’s original design in certain areas, such as calculateTokenAmount, getAdminBalance, and stopRampA. These differences were intentional design choices made to optimize performance and improve usability for our specific use case. For example, returning a positive amount when the total supply is zero in calculateTokenAmount simplifies initial liquidity provision without affecting pool state or security. Similarly, our additional check in stopRampA ensures that transitions are handled more safely without introducing any negative side effects.

In the addLiquidity and removeLiquidity functions of the Router contract, user-passed arrays (meta_amounts, base_amounts, etc.) are used to perform token transfers and allowances. If these arrays contain duplicate token addresses, the functions may perform redundant operations on the same token, leading to potential mismanagement of allowances or incorrect token balances. This could cause inconsistencies in the expected behavior of these operations, making it crucial to ensure that each token is processed only once.

Implement a validation mechanism to check for duplicate token addresses in the input arrays within addLiquidity and removeLiquidity functions. This will ensure that operations are executed accurately, with each token being handled only once, maintaining consistency and preventing any potential mismanagement of token balances.

RISK ACCEPTED: The BitFlux team accepted this risk of this finding and stated the following:

We acknowledge that allowing duplicate token entries in liquidity functions could theoretically lead to handling errors. However, our contract’s internal logic ensures that duplicate entries are processed correctly without causing unexpected behavior. This is a design decision to handle each token individually, even if duplicates exist in the input array, we make sure that all tokens are accounted for properly during liquidity operations.

The Router contract includes functions such as convert, addLiquidity, removeLiquidity, and removeBaseLiquidityOneToken that handle ERC20 token transfers. Although the use of transfer and transferFrom functions is common, it is important to consider the risk of reentrancy when these functions make multiple external calls. Even though standard ERC20 tokens are assumed to be secure, there could still be potential vulnerabilities if reentrancy protection is not implemented. This observation is informational, as no immediate issues are present given that only valid ERC20 tokens are used. However, it highlights the importance of adopting protective measures as a best practice.

It is recommended to implement ReentrancyGuard from OpenZeppelin’s library by adding the nonReentrant modifier to the mentioned function to prevent recursive calls.

ACKNOWLEDGED: The BitFlux team acknowledged this issue and stated the following:

Reentrancy protection is indeed crucial for preventing malicious recursive calls. We have already implemented reentrancy protection using OpenZeppelin’s ReentrancyGuard in critical areas where external calls are made. However, for other functions where reentrancy risks are minimal or non-existent (e.g., pure view functions or internal operations), we have opted not to apply reentrancy protection to avoid unnecessary gas costs.

In multiple functions across the codebase, the for loops use i++ (post-increment) for incrementing the loop counter. In Solidity, post-increment (i++) is slightly less efficient than pre-increment (++i) because i++ requires storing the original value of i in a temporary variable before incrementing, which consumes more gas. Although the gas difference is minimal, especially in recent Solidity versions, it becomes noticeable in larger loops or frequently executed functions, leading to inefficiencies in contract execution. The affected functions are the following:

• Router.convert

• Router.addLiquidity

• Router.removeLiquidity

• Router.calculateConvert

• Swap.initialize

• Swap.getAdminBalances

• SwapUtils.calculateWithdrawOneTokenDY

• SwapUtils.getYD

• SwapUtils.getD

• SwapUtils._xp

• SwapUtils.getY

• SwapUtils._calculateRemoveLiquidity

• SwapUtils.calculateTokenAmount

• SwapUtils.addLiquidity

• SwapUtils.removeLiquidity

• SwapUtils.removeLiquidityImbalance

• SwapUtils.withdrawAdminFees

To optimize gas usage, especially when iterating over large arrays or loops, it is recommended to replace i++ with ++i. Pre-increment (++i) does not require storing the old value of i, making it slightly more efficient in terms of gas consumption.

ACKNOWLEDGED: The BitFlux team acknowledged this issue and stated the following:

We acknowledge that using post-increment (i++) instead of pre-increment (++i) can lead to marginally higher gas usage. However, this difference is negligible in practice and does not significantly impact overall gas efficiency. Given that post-increment is more widely used and understood by developers, we have chosen to prioritize readability and consistency across our codebase over micro-optimizations.