Skip to content
This repository has been archived by the owner on Apr 28, 2024. It is now read-only.

Bauer - Incorrect implementation of checking whether borrowing collateral exceeds the maximum allowed collateral limit #72

Closed
sherlock-admin2 opened this issue Oct 23, 2023 · 1 comment
Assignees
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Oct 23, 2023

Bauer

high

Incorrect implementation of checking whether borrowing collateral exceeds the maximum allowed collateral limit

Summary

There is a misalignment between the cache.borrowedAmount and cache.holdTokenBalance variables. The former is derived from a pool's single-sided position, while the latter represents the quantity of hold tokens obtained after converting all liquidity. Consequently, the subtraction uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance may result in unexpected behavior, as it assumes these two variables can be directly compared, which may not be the case.

Vulnerability Detail

The function LiquidityBorrowingManager.borrow() handles the borrowing process by precalculating various details, initializing borrowing information, updating related data structures, checking collateral limits, transferring tokens. Inside the function, it checks if borrowing collateral exceeds the maximum allowed collateral.

 uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance;
        (borrowingCollateral > params.maxCollateral).revertError(
            ErrLib.ErrorCode.TOO_BIG_COLLATERAL
        );

The cache.borrowedAmount is calculated from a pool's single side position.

 function _getSingleSideRoundUpBorrowedAmount(
        bool zeroForSaleToken,
        int24 tickLower,
        int24 tickUpper,
        uint128 liquidity
    ) private pure returns (uint256 borrowedAmount) {
        borrowedAmount = (
            zeroForSaleToken
                ? LiquidityAmounts.getAmount1ForLiquidity(
                    TickMath.getSqrtRatioAtTick(tickLower),
                    TickMath.getSqrtRatioAtTick(tickUpper),
                    liquidity
                )
                : LiquidityAmounts.getAmount0ForLiquidity(
                    TickMath.getSqrtRatioAtTick(tickLower),
                    TickMath.getSqrtRatioAtTick(tickUpper),
                    liquidity
                )
        );
        if (borrowedAmount > Constants.MINIMUM_BORROWED_AMOUNT) {
            ++borrowedAmount;
        } else {
            revert TooLittleBorrowedLiquidity(liquidity);
        }
    }

The cache.holdTokenBalance is calculated as follows:
The protocol first calls the _decreaseLiquidity() function to reduce liquidity, which results in obtaining a certain amount of sale tokens and hold tokens.

function _decreaseLiquidity(uint256 tokenId, uint128 liquidity) private {
        // Call the decreaseLiquidity function of underlyingPositionManager contract
        // with DecreaseLiquidityParams struct as argument
        (uint256 amount0, uint256 amount1) = underlyingPositionManager.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: tokenId,
                liquidity: liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );
        // Check if both amount0 and amount1 are zero after decreasing liquidity
        // If true, revert with InvalidBorrowedLiquidity exception
        if (amount0 == 0 && amount1 == 0) {
            revert InvalidBorrowedLiquidity(tokenId);
        }
        // Call the collect function of underlyingPositionManager contract
        // with CollectParams struct as argument
        (amount0, amount1) = underlyingPositionManager.collect(
            INonfungiblePositionManager.CollectParams({
                tokenId: tokenId,
                recipient: address(this),
                amount0Max: uint128(amount0),
                amount1Max: uint128(amount1)
            })
        );
    }

Then, the sale tokens acquired in the previous step are exchanged for hold tokens. The result of this exchange, combined with the existing hold tokens that the borrower already had, constitutes the cache.holdTokenBalance.

 if (saleTokenBalance > 0) {
            if (params.externalSwap.swapTarget != address(0)) {
                // Call the external swap function and update the hold token balance in the cache
                cache.holdTokenBalance += _patchAmountsAndCallSwap(
                    params.saleToken,
                    params.holdToken,
                    params.externalSwap,
                    saleTokenBalance,
                    0
                );
            } else {
                // Call the internal v3SwapExactInput function and update the hold token balance in the cache
                cache.holdTokenBalance += _v3SwapExactInput(
                    v3SwapExactInputParams({
                        fee: params.internalSwapPoolfee,
                        tokenIn: params.saleToken,
                        tokenOut: params.holdToken,
                        amountIn: saleTokenBalance,
                        amountOutMinimum: 0
                    })
                );
            }
        }

The cache.borrowedAmount is calculated based on a pool's single-sided position, whereas cache.holdTokenBalance represents the quantity of hold tokens obtained after converting all liquidity. As a result, the subtraction uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance could lead to an unexpected failure.

Impact

The subtraction uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance could lead to an unexpected failure.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L492

Tool used

Manual Review

Recommendation

Change the code to uint256 borrowingCollateral = - cache.holdTokenBalance - cache.borrowedAmount

Duplicate of #86

@fann95 fann95 self-assigned this Oct 24, 2023
@fann95 fann95 added High A valid High severity issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Oct 24, 2023
@github-actions github-actions bot added the Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label label Oct 26, 2023
@sherlock-admin2 sherlock-admin2 changed the title Colossal Tan Hyena - Incorrect implementation of checking whether borrowing collateral exceeds the maximum allowed collateral limit Bauer - Incorrect implementation of checking whether borrowing collateral exceeds the maximum allowed collateral limit Oct 30, 2023
@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Oct 30, 2023
@fann95 fann95 removed High A valid High severity issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed Reward A payout will be made for this issue labels Nov 2, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout labels Nov 2, 2023
@sleepriverfish
Copy link

Escalate

This question has been marked as a duplicate of Question 86, but there is no reward. Both describe an issue with the line of code uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance. There is a potential overflow problem, and even the suggested fix is the same. This might be due to a typographical error, where an extra "-" has been added after the equals sign.

@Evert0x Evert0x added the Medium A valid Medium severity issue label Nov 13, 2023
@sherlock-admin sherlock-admin added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

5 participants