ParaSpace contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol#L249-L277

Vulnerability details

Impact

The oracleData calculation takes into account the difference between token0 and token1 decimals. But the current implementation is inconsistent. If the lp pair has 2 different decimal tokens, the returned price could be way off.

Proof of Concept

In the case token1Decimal > token0Decimal, the denominator is 1E9, but if token1Decimal < token0Decimal, the denominator becomes 10 ** (9 + oracleData.token0Decimal - oracleData.token1Decimal). The first treatment seems to be right, since the decimal difference is included in the nominator.

File: paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol
221:     function _getOracleData(UinswapV3PositionData memory positionData)
222:         internal
223:         view
224:         returns (PairOracleData memory)
225:     {

249:         } else if (oracleData.token1Decimal > oracleData.token0Decimal) {
250:             // multiple by 10^(decimalB - decimalA) to preserve price in wei
251:             oracleData.sqrtPriceX96 = uint160(
252:                 (SqrtLib.sqrt(
253:                     (oracleData.token0Price *
254:                         (10 **
255:                             (18 +
256:                                 oracleData.token1Decimal -
257:                                 oracleData.token0Decimal))) /
258:                         (oracleData.token1Price)
259:                 ) * 2**96) / 1E9
260:             );
261:         } else {
262:             // multiple by 10^(decimalA - decimalB) to preserve price in wei then divide by the same number
263:             oracleData.sqrtPriceX96 = uint160(
264:                 (SqrtLib.sqrt(
265:                     (oracleData.token0Price *
266:                         (10 **
267:                             (18 +
268:                                 oracleData.token0Decimal -
269:                                 oracleData.token1Decimal))) /
270:                         (oracleData.token1Price)
271:                 ) * 2**96) /
272:                     10 **
273:                         (9 +
274:                             oracleData.token0Decimal -
275:                             oracleData.token1Decimal)
276:             );
277:         }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Change the last else block to:

        } else {
            // multiple by 10^(decimalA - decimalB) to preserve price in wei then divide by the same number
            oracleData.sqrtPriceX96 = uint160(
                (SqrtLib.sqrt(
                    (oracleData.token0Price *
                        (10 **
                            (18 +
                                oracleData.token0Decimal -
                                oracleData.token1Decimal))) /
                        (oracleData.token1Price)
                ) * 2**96) / 1E9
            );
        }

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol#L156-L181

Vulnerability details

Impact

The total value locked sum of the pair is used to price the LP. But the reserves of the underlying can be easily influenced by flashloan, then the TVL can vary dramatically.Just like what happened before here Warp.

Proof of Concept

The price of the LP pair is determined by the TVL of the pool, given by: amt0 * price0 + amt1 * price1.

File: paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol
156:     function getTokenPrice(uint256 tokenId) public view returns (uint256) {
157:         UinswapV3PositionData memory positionData = getOnchainPositionData(
158:             tokenId
159:         );
160: 
161:         PairOracleData memory oracleData = _getOracleData(positionData);
162: 
163:         (uint256 liquidityAmount0, uint256 liquidityAmount1) = LiquidityAmounts
164:             .getAmountsForLiquidity(
165:                 oracleData.sqrtPriceX96,
166:                 TickMath.getSqrtRatioAtTick(positionData.tickLower),
167:                 TickMath.getSqrtRatioAtTick(positionData.tickUpper),
168:                 positionData.liquidity
169:             );
170: 
171:         (
172:             uint256 feeAmount0,
173:             uint256 feeAmount1
174:         ) = getLpFeeAmountFromPositionData(positionData);
175: 
176:         return
177:             (((liquidityAmount0 + feeAmount0) * oracleData.token0Price) /
178:                 10**oracleData.token0Decimal) +
179:             (((liquidityAmount1 + feeAmount1) * oracleData.token1Price) /
180:                 10**oracleData.token1Decimal);
181:     }

However, when a malicious user dumps large amount of any token into the pool, the whole TVL will be significantly increased, which leads to improper calculation of the price.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Update the TWAP LP price according to the "fair LP pricing" formula mentioned below:

• "Pricing LP tokens"
• "Fair Uniswap's LP Token Pricing"

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L362-L367 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L404-L407

Vulnerability details

Impact

For a newly added asset, the first price is always accepted without the requirement for MIN_ORACLES_NUM or price deviation check. Hence if some feeder intended to abuse the system, or in case of some feeder get compromised, the NFT floor oracle could be utilized to return distort price. And the protocol could loss fund due to the wrong price.

Proof of Concept

Newly added asset has no prior price _priorTwap or _updatedAt. In setPrice(), the 2 checks are in _checkValidity() and _combine().

File: paraspace-core/contracts/misc/NFTFloorOracle.sol
351:     function _checkValidity(address _asset, uint256 _twap) {

361:         //first price is always valid
362:         if (_priorTwap == 0 || _updatedAt == 0) {
363:             return true;
364:         }

397:     function _combine(address _asset, uint256 _twap) {

404:         //first time just use the feeding value
405:         if (assetPriceMap[_asset].twap == 0) {
406:             return (true, _twap);
407:         }

A malicious or compromised feeder can monitor the new event of added asset, and immediately set the price abnormally high. Then by using the underlying NFT, the feeder can take loan way above the market price. The pool has to take the fund loss.

But in fact, it can be mitigated by just require the same number of feeds for new assets.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Enforce the MIN_ORACLES_NUM requirement even if no prior price is set.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol#L152-L155 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L127-L129

Vulnerability details

Impact

In calculateUserAccountData(), each reserve needs to call external oracle for price feed. However, if any single one of the external oracle fails, the whole function will revert, causing DoS. The impacts could be the following:

• user fund lock
• contract inoperable, borrow/liquidation won't work
• pool could loss fund due to liquidation not working

Proof of Concept

In calculateUserAccountData(), every reserve will call external oracle for price feed.

File: paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol
74:     function calculateUserAccountData() {

152:                 vars.assetPrice = _getAssetPrice(
153:                     params.oracle,
154:                     vars.currentReserveAddress
155:                 );

But latestAnswer() in Chainlink is depreciated, in the future it could stop the service anytime.

File: paraspace-core/contracts/misc/ParaSpaceOracle.sol
115:     function getAssetPrice(address asset)
116:         public
117:         view
118:         override
119:         returns (uint256)
120:     {
121:         if (asset == BASE_CURRENCY) {
122:             return BASE_CURRENCY_UNIT;
123:         }
124: 
125:         uint256 price = 0;
126:         IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
127:         if (address(source) != address(0)) {
128:             price = uint256(source.latestAnswer());
129:         }
130:         if (price == 0 && address(_fallbackOracle) != address(0)) {
131:             price = _fallbackOracle.getAssetPrice(asset);
132:         }

Tools Used

Manual analysis.

Recommended Mitigation Steps

• use try/catch for potential external call failure
• skip the failed reserves and not include them in the account balance calculation

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L127-L132

Vulnerability details

Impact

For ERC20, according to Chainlink's documentation, the latestAnswer() function is deprecated.

The following could happen:

• For ERC20, DoS due to deprecated API stops working. Prices cannot be obtained, if Chainlink stopped supporting deprecated APIs, and _fallbackOracle is not set.
• _fallbackOracle uses uniswap v2 getAmountOut(), it could be manipulated. Malicious user could monitor the Chainlink API, and start the price manipulation right after to steal fund from Paraspace.

Proof of Concept

File: paraspace-core/contracts/misc/ParaSpaceOracle.sol
127:         if (address(source) != address(0)) {
128:             price = uint256(source.latestAnswer());
129:         }
130:         if (price == 0 && address(_fallbackOracle) != address(0)) {
131:             price = _fallbackOracle.getAssetPrice(asset);
132:         }

But the _fallbackOracle uses uniswap v2 function getAmountOut() to return the price, which is vulnerable to manipulation. A malicious user could monitor the Chainlink support of the API, as soon as the service is not available, the uniswap v2 lp manipulation could follow immediately.

Reference:

Chainlink - latestanswer

Recommended Mitigation Steps

• just revert if the ERC20 oracle is not available, instead of using single getAmountOut()
• consider using TWAP price
• use the latestRoundData() function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is incomplete, for example:

    (uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = source.latestRoundData();
    require(answeredInRound >= roundID, "...");
    require(timeStamp != 0, "...");
    require(price != 0, "...");

unused import

ERC20 lib in ERC721 contract.

File: paraspace-core/contracts/protocol/tokenization/NToken.sol
20: import {SafeERC20} from "../../dependencies/openzeppelin/contracts/SafeERC20.sol";

Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified.

Instances number of this issue:

File: paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol
149:     receive() external payable {}
150: }

Events not emitted for important state changes

When changing critical state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Instances number of this issue: 1

File: protocol/configuration/PoolAddressesProvider.sol
56:     function setMarketId(string memory newMarketId)
57:         external
58:         override
59:         onlyOwner
60:     {
61:         _setMarketId(newMarketId);
62:     }

Suggestion: Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity.

repay on behalf of other user in full

Currently it does not allow repay in full on behalf of other users.

File: paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol
378:         require(
379:             amountSent != type(uint256).max || msg.sender == onBehalfOf,
380:             Errors.NO_EXPLICIT_AMOUNT_TO_REPAY_ON_BEHALF
381:         );

However, from business logic it does not seem to have any bad effects to allow repaying in full for others.

Suggestion: Allow repay for others in full.