This repository has been archived by the owner on Dec 17, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
1bede8c
commit 56ee002
Showing
42 changed files
with
1,518 additions
and
458 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,42 +1,21 @@ | ||
| p0wd3r | ||
| IceBear | ||
|
|
||
| medium | ||
|
|
||
| # Obtaining asset prices from Oracle is not applicable for tokens with decimals greater than 18. | ||
| # Lack of slippage control | ||
|
|
||
| ## Summary | ||
| Obtaining asset prices from Oracle is not applicable for tokens with decimals greater than 18. | ||
| In UniswapExtension.sol, the functions uniV3ExactOutputInternal() and uniV3ExactInputInternal() lack slippage control. | ||
| ## Vulnerability Detail | ||
| Assumed decimal <= 18 directly in `getNormalizedPrice` and `getPriceFromChainlink`, if decimal > 18 it will overflow and cause price calculation errors. | ||
|
|
||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L80 | ||
| ```solidity | ||
| function getNormalizedPrice(uint256 price, address asset) internal view returns (uint256) { | ||
| uint8 decimals = IERC20Metadata(asset).decimals(); | ||
| return price * 10 ** (18 - decimals); | ||
| } | ||
| ``` | ||
|
|
||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L66 | ||
| ```solidity | ||
| function getPriceFromChainlink(address base, address quote) internal view returns (uint256) { | ||
| (, int256 price,,,) = registry.latestRoundData(base, quote); | ||
| require(price > 0, "invalid price"); | ||
| // Extend the decimals to 1e18. | ||
| return uint256(price) * 10 ** (18 - uint256(registry.decimals(base, quote))); | ||
| } | ||
| ``` | ||
| Calling uniV3ExactOutputInternal() and uniV3ExactInputInternal() can lead to slippage. | ||
| ## Impact | ||
| Unable to obtain the correct price for tokens with decimals greater than 18. | ||
| The slippage control is set to the maximum and minimum of the allowed max and min sqrt ratio,without precise slippage control, swaps done by user are susceptible to sandwich attacks. | ||
| ## Code Snippet | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L80 | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L66 | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/UniswapExtension.sol#L715-L736 | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/UniswapExtension.sol#L744-L761 | ||
| ## Tool used | ||
|
|
||
| Manual Review | ||
|
|
||
| ## Recommendation | ||
| Check if the decimal is greater than 18 and take appropriate action, or limit the decimal to <= 18 in `_setAggregators`. | ||
|
|
||
| Chainlink's advice for this: https://docs.chain.link/data-feeds/examples#getting-a-different-price-denomination | ||
| Recommend following uniswap's way of implementing the expected minimum amount out check. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,31 @@ | ||
| IceBear | ||
| rvierdiiev | ||
|
|
||
| medium | ||
|
|
||
| # Lack of slippage control | ||
| # PriceOracle.getPrice doesn't check for stale price | ||
|
|
||
| ## Summary | ||
| In UniswapExtension.sol, the functions uniV3ExactOutputInternal() and uniV3ExactInputInternal() lack slippage control. | ||
| PriceOracle.getPrice doesn't check for stale price. As result protocol can make decisions based on not up to date prices, which can cause loses. | ||
| ## Vulnerability Detail | ||
| Calling uniV3ExactOutputInternal() and uniV3ExactInputInternal() can lead to slippage. | ||
| `PriceOracle.getPrice` function is going to provide asset price using chain link price feeds. | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/oracle/PriceOracle.sol#L66-L72 | ||
| ```soldiity | ||
| function getPriceFromChainlink(address base, address quote) internal view returns (uint256) { | ||
| (, int256 price,,,) = registry.latestRoundData(base, quote); | ||
| require(price > 0, "invalid price"); | ||
| // Extend the decimals to 1e18. | ||
| return uint256(price) * 10 ** (18 - uint256(registry.decimals(base, quote))); | ||
| } | ||
| ``` | ||
| This function doesn't check that prices are up to date. Because of that it's possible that price is not outdated which can cause financial loses for protocol. | ||
| ## Impact | ||
| The slippage control is set to the maximum and minimum of the allowed max and min sqrt ratio,without precise slippage control, swaps done by user are susceptible to sandwich attacks. | ||
| Protocol can face bad debt. | ||
| ## Code Snippet | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/UniswapExtension.sol#L715-L736 | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/UniswapExtension.sol#L744-L761 | ||
| Provided above | ||
| ## Tool used | ||
|
|
||
| Manual Review | ||
|
|
||
| ## Recommendation | ||
| Recommend following uniswap's way of implementing the expected minimum amount out check. | ||
| You need to check that price is not outdated by checking round timestamp. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,52 +1,55 @@ | ||
| rvierdiiev | ||
| kutugu | ||
|
|
||
| medium | ||
|
|
||
| # IronBank._isLiquidatable should acrrue interests for all user's markets in order to receive correct collateral/debt balance | ||
| # SafeApprove require allowance is zero | ||
|
|
||
| ## Summary | ||
| IronBank._isLiquidatable should acrrue interests for all user's markets in order to receive correct collateral/debt balance. Because it's not done now, that means that user can be liquidated when he actually has enough collateral. | ||
|
|
||
| SafeApprove require allowance is zero, should use `forceApprove` instead of `safeApprove`. | ||
|
|
||
| ## Vulnerability Detail | ||
| `_isLiquidatable` function is used by protocol in order to understand if user has enough collateral to cover his debt. To do that function loops through all user's markets. | ||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/protocol/pool/IronBank.sol#L1065-L1092 | ||
|
|
||
| For flashLoan, when repay contract will approve ironBank. | ||
| ```solidity | ||
| function _isLiquidatable(address user) internal view returns (bool) { | ||
| uint256 liquidationCollateralValue; | ||
| uint256 debtValue; | ||
| address[] memory userEnteredMarkets = allEnteredMarkets[user]; | ||
| for (uint256 i = 0; i < userEnteredMarkets.length; i++) { | ||
| DataTypes.Market storage m = markets[userEnteredMarkets[i]]; | ||
| if (!m.config.isListed) { | ||
| continue; | ||
| } | ||
| uint256 supplyBalance = m.userSupplies[user]; | ||
| uint256 borrowBalance = _getBorrowBalance(m, user); | ||
| uint256 assetPrice = PriceOracleInterface(priceOracle).getPrice(userEnteredMarkets[i]); | ||
| require(assetPrice > 0, "invalid price"); | ||
| uint256 liquidationThreshold = m.config.liquidationThreshold; | ||
| if (supplyBalance > 0 && liquidationThreshold > 0) { | ||
| uint256 exchangeRate = _getExchangeRate(m); | ||
| liquidationCollateralValue += | ||
| (supplyBalance * exchangeRate * assetPrice * liquidationThreshold) / 1e36 / FACTOR_SCALE; | ||
| } | ||
| if (borrowBalance > 0) { | ||
| debtValue += (borrowBalance * assetPrice) / 1e18; | ||
| } | ||
| IERC20(token).safeTransferFrom(address(receiver), address(this), amount); | ||
| uint256 allowance = IERC20(token).allowance(address(this), ironBank); | ||
| if (allowance < amount) { | ||
| IERC20(token).safeApprove(ironBank, type(uint256).max); | ||
| } | ||
| return debtValue > liquidationCollateralValue; | ||
| IronBankInterface(ironBank).repay(address(this), address(this), token, amount); | ||
| ``` | ||
| It starts with allowance of 0, which is less than repay amount, so it will approve ironBank `type(uint256).max`. | ||
| Over a long period of flashloan, allowance is gradually reduced until it is eventually less than the repay amount, but greater than 0. Note that flashloan only limits tokens to market ERC20 tokens(any vanilla ERC20s), it does not specify `transferFrom` won't reduce an allowance of `type(uint256).max`. | ||
| When `safeApprove` is called again, due to the fact that allowance is not 0, the call will revert, and this token cannot be loaned thereafter, so it can be considered as permanent dos. | ||
| ```solidity | ||
| function safeApprove(IERC20 token, address spender, uint256 value) internal { | ||
| // safeApprove should only be called when setting an initial allowance, | ||
| // or when resetting it to zero. To increase and decrease it, use | ||
| // 'safeIncreaseAllowance' and 'safeDecreaseAllowance' | ||
| require( | ||
| (value == 0) || (token.allowance(address(this), spender) == 0), | ||
| "SafeERC20: approve from non-zero to non-zero allowance" | ||
| ); | ||
| _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value)); | ||
| } | ||
| ``` | ||
| As you can see this function doesn't accrue interests for all user's markets. That means that collateral balance and debt balance could be wrong for that markets, because accruing interests has impact on both exchange ratio and borrow index. | ||
| Of course since `type(uint256).max` is a huge number which is fine for most tokens for a long time. But note again that the `totalSupply` of market tokens can be large, which has no limit. | ||
|
|
||
| ## Impact | ||
| User can be liquidated, when he has enough collateral. | ||
|
|
||
| Medium. If allowance is reduced to less than repay amount, flashloan this market token is not available. | ||
|
|
||
| ## Code Snippet | ||
| Provided above | ||
|
|
||
| https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/flashLoan/FlashLoan.sol#L108 | ||
|
|
||
| ## Tool used | ||
|
|
||
| Manual Review | ||
|
|
||
| ## Recommendation | ||
| You need to accrue interests for all markets of user. | ||
|
|
||
| use `forceApprove` instead of `safeApprove`. |
Oops, something went wrong.