You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on Apr 28, 2024. It is now read-only.
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelMediumA valid Medium severity issueRewardA payout will be made for this issue
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.
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(uint256tokenId, uint128liquidity) private {
// Call the decreaseLiquidity function of underlyingPositionManager contract// with DecreaseLiquidityParams struct as argument
(uint256amount0, uint256amount1) = 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 exceptionif (amount0 ==0&& amount1 ==0) {
revertInvalidBorrowedLiquidity(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.
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
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.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
DuplicateA valid issue that is a duplicate of an issue with `Has Duplicates` labelMediumA valid Medium severity issueRewardA payout will be made for this issue
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.The
cache.borrowedAmount
is calculated from a pool's single side position.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.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.
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
The text was updated successfully, but these errors were encountered: