Boot Finance contest - jonah1005's results

Handle

jonah1005

Vulnerability details

Impact

The sanity checks in rampTargetPrice are broken SwapUtils.sol#L1571-L1581

        if (futureTargetPricePrecise < initialTargetPricePrecise) {
            require(
                futureTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE).div(WEI_UNIT) >= initialTargetPricePrecise,
                "futureTargetPrice_ is too small"
            );
        } else {
            require(
                futureTargetPricePrecise <= initialTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE).div(WEI_UNIT),
                "futureTargetPrice_ is too large"
            );
        }

If futureTargetPricePrecise is smaller than initialTargetPricePrecise 0.01 of futureTargetPricePrecise would never larger than initialTargetPricePrecise.

Admin would not be able to ramp the target price. As it's one of the most important features of the customswap, I consider this is a high-risk issue

Proof of Concept

Here's a web3.py script to demo that it's not possible to change the target price even by 1 wei.

    p1, p2, _, _ =swap.functions.targetPriceStorage().call()
    future = w3.eth.getBlock(w3.eth.block_number)['timestamp'] + 200 * 24 * 3600

    # futureTargetPrice_ is too small
    swap.functions.rampTargetPrice(p1 -1, future).transact()
    # futureTargetPrice_ is too large
    swap.functions.rampTargetPrice(p1 + 1, future).transact()

Tools Used

None

Recommended Mitigation Steps

Would it be something like:

        if (futureTargetPricePrecise < initialTargetPricePrecise) {
            require(
                futureTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE + WEI_UNIT).div(WEI_UNIT) >= initialTargetPricePrecise,
                "futureTargetPrice_ is too small"
            );
        } else {
            require(
                futureTargetPricePrecise <= initialTargetPricePrecise.mul(MAX_RELATIVE_PRICE_CHANGE + WEI_UNIT).div(WEI_UNIT),
                "futureTargetPrice_ is too large"
            );
        }

I believe the dev would spot this mistake if there's a more relaxed timeline.

Handle

jonah1005

Vulnerability details

Impact

When a user provides imbalanced liquidity, the fee is calculated according to the ideal balance. In saddle finance, the optimal balance should be the same ratio as in the Pool.

Take, for example, if there's 10000 USD and 10000 DAI in the saddle's USD/DAI pool, the user should get the optimal lp if he provides lp with ratio = 1.

However, if the customSwap pool is created with a target price = 2. The user would get 2 times more lp if he deposits DAI. SwapUtils.sol#L1227-L1245 The current implementation does not calculates ideal balance correctly.

If the target price is set to be 10, the ideal balance deviates by 10. The fee deviates a lot. I consider this is a high-risk issues.

Proof of Concept

We can observe the issue if we initiates two pools DAI/LINK pool and set the target price to be 4.

For the first pool, we deposit more dai.

    swap = deploy_contract('Swap' 
        [dai.address, link.address], [18, 18], 'lp', 'lp', 1, 85, 10**7, 0, 0, 4* 10**18)
    link.functions.approve(swap.address, deposit_amount).transact()
    dai.functions.approve(swap.address, deposit_amount).transact()
    previous_lp = lptoken.functions.balanceOf(user).call()
    swap.functions.addLiquidity([deposit_amount, deposit_amount // 10], 10, 10**18).transact()
    post_lp = lptoken.functions.balanceOf(user).call()
    print('get lp', post_lp - previous_lp)

For the second pool, one we deposit more dai.

    swap = deploy_contract('Swap' 
        [dai.address, link.address], [18, 18], 'lp', 'lp', 1, 85, 10**7, 0, 0, 4* 10**18)
    link.functions.approve(swap.address, deposit_amount).transact()
    dai.functions.approve(swap.address, deposit_amount).transact()
    previous_lp = lptoken.functions.balanceOf(user).call()
    swap.functions.addLiquidity([deposit_amount, deposit_amount // 10], 10, 10**18).transact()
    post_lp = lptoken.functions.balanceOf(user).call()
    print('get lp', post_lp - previous_lp)

We can get roughly 4x more lp in the first case

Tools Used

None

Recommended Mitigation Steps

The current implementation uses self.balances

https://github.com/code-423n4/2021-11-bootfinance/blob/main/customswap/contracts/SwapUtils.sol#L1231-L1236

            for (uint256 i = 0; i < self.pooledTokens.length; i++) {
                uint256 idealBalance = v.d1.mul(self.balances[i]).div(v.d0);
                fees[i] = feePerToken
                    .mul(idealBalance.difference(newBalances[i]))
                    .div(FEE_DENOMINATOR);
                self.balances[i] = newBalances[i].sub(
                    fees[i].mul(self.adminFee).div(FEE_DENOMINATOR)
                );
                newBalances[i] = newBalances[i].sub(fees[i]);
            }

Replaces self.balances with _xp(self, newBalances) would be a simple fix. I consider the team can take balance's weighted pool as a reference. WeightedMath.sol#L149-L179

Handle

jonah1005

Vulnerability details

Impact

CustomPrecisionMultipliers are set in the constructor:

        customPrecisionMultipliers[0] = targetPriceStorage.originalPrecisionMultipliers[0].mul(_targetPrice).div(10 ** 18);

originalPrecisionMultipliers equal to 1 if the token's decimal = 18. The targe price could only be an integer.

If the target price is bigger than 10**18, the user can deposit and trade in the pool. Though, the functionality would be far from the spec.

If the target price is set to be smaller than 10**18, the pool would be broken and all funds would be stuck.

I consider this is a high-risk issue.

Proof of Concept

Please refer to the implementation. Swap.sol#L184-L187

We can also trigger the bug by setting a pool with target price = 0.5. (0.5 * 10**18)

Tools Used

None

Recommended Mitigation Steps

I recommend providing extra 10**18 in both multipliers.

        customPrecisionMultipliers[0] = targetPriceStorage.originalPrecisionMultipliers[0].mul(_targetPrice).mul(10**18).div(10 ** 18);
        customPrecisionMultipliers[1] = targetPriceStorage.originalPrecisionMultipliers[1].mul(10**18);

The customswap only supports two tokens in a pool, there's should be enough space. Recommend the devs to go through the trade-off saddle finance has paid to support multiple tokens. The code could be more clean and efficient if the pools' not support multiple tokens.

Handle

jonah1005

Vulnerability details

Impact

There's a feature of virtualPrice that is monotonically increasing regardless of the market. This function is heavily used in multiple protocols. e.g.(curve metapool, mim, ...) This is not held in the current implementation of customSwap since customPrecisionMultipliers can be changed by changing the target price.

There're two issues here: The meaning of virtualPrice would be vague. This may damage the lp providers as the protocol that adopts it may be hacked.

I consider this is a medium-risk issue.

Proof of Concept

We can set up a mockSwap with extra setPrecisionMultiplier to check the issue.

    function setPrecisionMultiplier(uint256 multipliers) external {
        swapStorage.tokenPrecisionMultipliers[0] = multipliers; 
    }

    print(swap.functions.getVirtualPrice().call())
    swap.functions.setPrecisionMultiplier(2).transact()
    print(swap.functions.getVirtualPrice().call())

# output log:
#   1000000000000000000
#   1499889859738721606

Tools Used

None

Recommended Mitigation Steps

Dealing with the target price with multiplier precision seems clever as we can reuse most of the existing code. However, the precision multiplier should be an immutable parameter. Changing it after the pool is set up would create multiple issues. This function could be implemented in a safer way IMHO.

The quick fix would be to remove the getVirtualPrice function. I can't come up with a safe way if other protocol wants to use this function.

Handle

jonah1005

Vulnerability details

Stop ramp target price would create huge arbitrage space.

Impact

stopRampTargetPrice would set the tokenPrecisionMultipliers to originalPrecisionMultipliers[0].mul(currentTargetPrice).div(WEI_UNIT); Once the tokenPrecisionMultipliers is changed, the price in the AMM pool would change. Arbitrager can sandwich stopRampTargetPrice to gain profit.

Assume the decision is made in the DAO, an attacker can set up the bot once the proposal to stopRampTargetPrice has passed. I consider this is a medium-risk issue.

Proof of Concept

The precisionMultiplier is set here: Swap.sol#L661-L666

We can set up a mockSwap with extra setPrecisionMultiplier to check the issue.

    function setPrecisionMultiplier(uint256 multipliers) external {
        swapStorage.tokenPrecisionMultipliers[0] = multipliers; 
    }

print(swap.functions.getVirtualPrice().call())
swap.functions.setPrecisionMultiplier(2).transact()
print(swap.functions.getVirtualPrice().call())

# output log:
#     1000000000000000000
#     1499889859738721606

Tools Used

None

Recommended Mitigation Steps

Dealing with the target price with multiplier precision seems clever as we can reuse most of the existing code. However, the precision multiplier should be an immutable parameter. Changing it after the pool is setup would create multiple issues. This function could be implemented in a safer way IMHO.

A quick fix I would come up with is to ramp the tokenPrecisionMultipliers as the aPrecise is ramped. As the tokenPrecision is slowly increased/decreased, the arbitrage space would be slower and the profit would (probably) distribute evenly to lpers.

Please refer to _getAPreceise's implementation SwapUtils.sol#L227-L250