ParaSpace contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceOracle.sol#L131 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol#L56 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol#L78

Vulnerability details

Impact

Fallback oracle is using spot price in Uniswap liquidity pool, which is very vulnerable to flashloan price manipulation. Hacker can use flashloan to distort the price and overborrow or perform malicious liqudiation.

Proof of Concept

In the current implementation of the paraspace oracle, if the paraspace oracle has issue, the fallback oracle is used for ERC20 token.

/// @inheritdoc IPriceOracleGetter
function getAssetPrice(address asset)
	public
	view
	override
	returns (uint256)
{
	if (asset == BASE_CURRENCY) {
		return BASE_CURRENCY_UNIT;
	}

	uint256 price = 0;
	IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
	if (address(source) != address(0)) {
		price = uint256(source.latestAnswer());
	}
	if (price == 0 && address(_fallbackOracle) != address(0)) {
		price = _fallbackOracle.getAssetPrice(asset);
	}

	require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
	return price;
}

which calls:

price = _fallbackOracle.getAssetPrice(asset);

whch use the spot price from Uniswap V2.

	address pairAddress = IUniswapV2Factory(UNISWAP_FACTORY).getPair(
		WETH,
		asset
	);
	require(pairAddress != address(0x00), "pair not found");
	IUniswapV2Pair pair = IUniswapV2Pair(pairAddress);
	(uint256 left, uint256 right, ) = pair.getReserves();
	(uint256 tokenReserves, uint256 ethReserves) = (asset < WETH)
		? (left, right)
		: (right, left);
	uint8 decimals = ERC20(asset).decimals();
	//returns price in 18 decimals
	return
		IUniswapV2Router01(UNISWAP_ROUTER).getAmountOut(
			10**decimals,
			tokenReserves,
			ethReserves
		);

and

function getEthUsdPrice() public view returns (uint256) {
	address pairAddress = IUniswapV2Factory(UNISWAP_FACTORY).getPair(
		USDC,
		WETH
	);
	require(pairAddress != address(0x00), "pair not found");
	IUniswapV2Pair pair = IUniswapV2Pair(pairAddress);
	(uint256 left, uint256 right, ) = pair.getReserves();
	(uint256 usdcReserves, uint256 ethReserves) = (USDC < WETH)
		? (left, right)
		: (right, left);
	uint8 ethDecimals = ERC20(WETH).decimals();
	//uint8 usdcDecimals = ERC20(USDC).decimals();
	//returns price in 6 decimals
	return
		IUniswapV2Router01(UNISWAP_ROUTER).getAmountOut(
			10**ethDecimals,
			ethReserves,
			usdcReserves
		);
}

Using flashloan to distort and manipulate the price is very damaging technique.

Consider the POC below.

1. the User uses 10000 amount of tokenA as collateral, each token A worth 1 USD according to the paraspace oracle. the user borrow 3 ETH, the price of ETH is 1200 USD.

2. the paraspace oracle went down, the fallback price oracle is used, the user use borrows flashloan to distort the price of the tokenA in Uniswap pool from 1 USD to 10000 USD.

3. the user's collateral position worth 1000 token X 10000 USD, and borrow 1000 ETH.

4. User repay the flashloan using the overborrowed amount and recover the price of the tokenA in Uniswap liqudity pool to 1 USD, leaving bad debt and insolvent position in Paraspace.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project does not use the spot price in Uniswap V2, if the paraspace is down, it is safe to just revert the transaction.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol#L428 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol#L439 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol#L74

Vulnerability details

Impact

Pending Apecoin staking reward does not count towards the account health

Proof of Concept

The account health factor is important because it is used to determine how much user can borrow and when the price drops, it is used to determine if the liquidation is allowed.

Let us look into the health factor calculation in GenericLogic

        CalculateUserAccountDataVars memory vars;

        while (vars.i < params.reservesCount) {
            if (!params.userConfig.isUsingAsCollateralOrBorrowing(vars.i)) {
                unchecked {
                    ++vars.i;
                }
                continue;
            }

            vars.currentReserveAddress = reservesList[vars.i];

            if (vars.currentReserveAddress == address(0)) {
                unchecked {
                    ++vars.i;
                }
                continue;
            }

            DataTypes.ReserveData storage currentReserve = reservesData[
                vars.currentReserveAddress
            ];

            vars.reserveConfiguration = currentReserve.configuration;

            (
                vars.ltv,
                vars.liquidationThreshold,
                vars.liquidationBonus,
                vars.decimals,

            ) = currentReserve.configuration.getParams();

            unchecked {
                vars.assetUnit = 10**vars.decimals;
            }

            vars.xTokenAddress = currentReserve.xTokenAddress;

            if (
                vars.reserveConfiguration.getAssetType() ==
                DataTypes.AssetType.ERC20
            ) {
                vars.assetPrice = _getAssetPrice(
                    params.oracle,
                    vars.currentReserveAddress
                );

                if (
                    (vars.liquidationThreshold != 0) &&
                    params.userConfig.isUsingAsCollateral(vars.i)
                ) {
                    vars.userBalanceInBaseCurrency = _getUserBalanceForERC20(
                        params.user,
                        currentReserve,
                        vars.xTokenAddress,
                        vars.assetUnit,
                        vars.assetPrice
                    );

                    vars.payableDebtByERC20Assets += vars
                        .userBalanceInBaseCurrency
                        .percentDiv(vars.liquidationBonus);

                    vars.liquidationThreshold =
                        vars.userBalanceInBaseCurrency *
                        (vars.liquidationThreshold);
                    vars.avgLtv += vars.userBalanceInBaseCurrency * vars.ltv;

                    vars.totalCollateralInBaseCurrency += vars
                        .userBalanceInBaseCurrency;

                    if (vars.ltv == 0) {
                        vars.hasZeroLtvCollateral = true;
                    }

                    vars.avgLiquidationThreshold += vars.liquidationThreshold;
                }

                if (params.userConfig.isBorrowing(vars.i)) {
                    vars.totalDebtInBaseCurrency += _getUserDebtInBaseCurrency(
                        params.user,
                        currentReserve,
                        vars.assetPrice,
                        vars.assetUnit
                    );
                }
            } else {
                if (
                    (vars.liquidationThreshold != 0) &&
                    params.userConfig.isUsingAsCollateral(vars.i)
                ) {
                    vars.xTokenType = INToken(vars.xTokenAddress)
                        .getXTokenType();
                    if (vars.xTokenType == XTokenType.NTokenUniswapV3) {
                        (
                            vars.userBalanceInBaseCurrency,
                            vars.ltv,
                            vars.liquidationThreshold
                        ) = _getUserBalanceForUniswapV3(
                            reservesData,
                            params,
                            vars
                        );
                    } else {
                        vars
                            .userBalanceInBaseCurrency = _getUserBalanceForERC721(
                            params,
                            vars
                        );

                        vars.liquidationThreshold =
                            vars.userBalanceInBaseCurrency *
                            vars.liquidationThreshold;

                        if (vars.ltv == 0) {
                            vars.hasZeroLtvCollateral = true;
                        }

                        vars.ltv = vars.userBalanceInBaseCurrency * vars.ltv;
                    }

                    vars.avgERC721LiquidationThreshold += vars
                        .liquidationThreshold;
                    vars.totalERC721CollateralInBaseCurrency += vars
                        .userBalanceInBaseCurrency;
                    vars.totalCollateralInBaseCurrency += vars
                        .userBalanceInBaseCurrency;
                    vars.avgLtv += vars.ltv;
                    vars.avgLiquidationThreshold += vars.liquidationThreshold;
                }
            }

            unchecked {
                ++vars.i;
            }
        }

        unchecked {
            vars.avgLtv = vars.totalCollateralInBaseCurrency != 0
                ? vars.avgLtv / vars.totalCollateralInBaseCurrency
                : 0;
            vars.avgLiquidationThreshold = vars.totalCollateralInBaseCurrency !=
                0
                ? vars.avgLiquidationThreshold /
                    vars.totalCollateralInBaseCurrency
                : 0;

            vars.avgERC721LiquidationThreshold = vars
                .totalERC721CollateralInBaseCurrency != 0
                ? vars.avgERC721LiquidationThreshold /
                    vars.totalERC721CollateralInBaseCurrency
                : 0;
        }

        vars.healthFactor = (vars.totalDebtInBaseCurrency == 0)
            ? type(uint256).max
            : (
                vars.totalCollateralInBaseCurrency.percentMul(
                    vars.avgLiquidationThreshold
                )
            ).wadDiv(vars.totalDebtInBaseCurrency);

        vars.erc721HealthFactor = (vars.totalDebtInBaseCurrency == 0 ||
            vars.payableDebtByERC20Assets >= vars.totalDebtInBaseCurrency)
            ? type(uint256).max
            : (
                vars.totalERC721CollateralInBaseCurrency.percentMul(
                    vars.avgERC721LiquidationThreshold
                )
            ).wadDiv(
                    vars.totalDebtInBaseCurrency - vars.payableDebtByERC20Assets
                );

        return (
            vars.totalCollateralInBaseCurrency, //a 
            vars.totalERC721CollateralInBaseCurrency, // b
            vars.totalDebtInBaseCurrency, // c
            vars.avgLtv, // d
            vars.avgLiquidationThreshold, // e
            vars.avgERC721LiquidationThreshold, // f
            vars.payableDebtByERC20Assets, // g
            vars.healthFactor, // 
            vars.erc721HealthFactor, // h
            vars.hasZeroLtvCollateral // i
        );
    }

this function is very long, but on the high level, what is does is that:

loop over all ERC20 collateral and calculate the borrow amount, the asset worth. loop over ERC721 token including Uniswap V3 NFT and other NFT supported.

the list of NFT supported according to the documentation is:

https://docs.para.space/para-space/introduction-to-paraspace/supplying-nfts/nft-utility-and-delegated-rights#what-other-benefits-does-paraspace-offer-to-nft-holders

Bored Apt, MoonBird, CryptoPunk, Mutant Ape, Doodles, Otherdeed, Clone x and meetbits.

The parapspace also support ape coin staking optimization.

but here is the issue: the code above that calculate the account health factor does not count the pending apecoin staking reward.

When apecoin launch the staking, this happens:

https://twitter.com/peckshieldalert/status/1599961388298166272

basically user stake their ape with ape coin for reward and they create a sell order to sell the NFT in NFT marketplace.

Another user happily step in, he first borrow flashloan, buy the NFT, claim the ape coin, then sold apecoin + the NFT for profits and repay the flashloan. It is user that create the sell order suffers: he loses the apecoin and the staking reward.

Now consider this case in the current paraspace account health factor implementation:

1. A User have a bored APE NFT, worth 80 ETH
2. He stakes the ape coin for his NFT, the ape coin staked worth 20 ETH.
3. The User use the bored APE NFT as collateral in the paraspace system and borrow 30 ETH.
4. The bored APE NFT price drops to 10 ETH, the system thinks the loan backed by the NFT is bad debt and the NFT is liquidated.
5. However, when the APE NFT worth NFT, his staked apecoin + the staking reward can worth 50 ETH, without counting the pending staking reward of the apecoin into the account health factor, the user's NFT is liquidated and the user lose his NFT + the apecoin.

Tools Used

Manual Review.

Recommended Mitigation Steps

We recommend the project let pending Apecoin staking reward count towards the total collateral value when calculating account health factor.

Unsafe Downcasting Operation

line of code.

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

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

vulnerability detail and recommended fix

The unsafe downcasting can truncated the arithmic value unexpected.

assetFeederMap[_asset].index = uint8(assets.length - 1);

and

feederPositionMap[_feeder].index = uint8(feeders.length - 1);

We recommend using Openzeppelin safeCast.

https://docs.openzeppelin.com/contracts/3.x/api/utils#SafeCast

Credit Signature never expires.

line of code.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol#L1117

vulnerability detail and recommended fix

The credit signature schema does not have expiration timestamp so the credit signature never expires.

function hashCredit(DataTypes.Credit memory credit)
	private
	pure
	returns (bytes32)
{
	bytes32 typeHash = keccak256(
		abi.encodePacked(
			"Credit(address token,uint256 amount,bytes orderId)"
		)
	);

	// https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#definition-of-encodedata
	return
		keccak256(
			abi.encode(
				typeHash,
				credit.token,
				credit.amount,
				keccak256(abi.encodePacked(credit.orderId))
			)
		);
}

the user can sign a credit order, and the order is never executed, then after 10 days, it is no longer profitable to execute the order or the user does not execute the order, but the signature does not expire.

This is QA because the Opensea order, looksrare and X2Y2 both has deadine and expiration time.

We recommend the project add nonce to make sure the signature cannot be reused and also add timestamp to make sure the credit signature has expiraton time.

UniswapV3OracleWrapper.sol latestAnswer() alwasy revert.

line of code.

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

vulnerability detail and recommended fix

below is the current implementation of the Paraspace oracle,

/// @inheritdoc IPriceOracleGetter
function getAssetPrice(address asset)
	public
	view
	override
	returns (uint256)
{
	if (asset == BASE_CURRENCY) {
		return BASE_CURRENCY_UNIT;
	}

	uint256 price = 0;
	IEACAggregatorProxy source = IEACAggregatorProxy(assetsSources[asset]);
	if (address(source) != address(0)) {
		price = uint256(source.latestAnswer());
	}
	if (price == 0 && address(_fallbackOracle) != address(0)) {
		price = _fallbackOracle.getAssetPrice(asset);
	}

	require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
	return price;
}

note that the oracle source is expected to implement source.latestAnswer, however, the UniswapV3OracleWrapper.sol does not implement this function.

function latestAnswer() external pure returns (int256) {
	revert("unimplemented");
}

We recommend the project implement latestAnswer function in UniswapV3Oracle to avoid transactoinr revert when using UniswapV3OracleWrapper.

For PoolApeStaking.sol: lack of method to preview the claimable reward amount.

line of code.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol#L23

vulnerability detail and recommended fix

There is no method to preview the claimable reward amount in PoolApeStaking.sol.

We recommend the project add the method to preview the claimable amount of reward to let user keep track of their position properly.

The method is

https://github.com/HorizenLabs/ape-staking-public/blob/6b86a6d92153455fe9f33b0bda16e4141099fc7d/contracts/ApeCoinStaking.sol#L897

function pendingRewards(uint256 _poolId, address _address, uint256 _tokenId) external view returns (uint256) {
	Pool memory pool = pools[_poolId];
	Position memory position = _poolId == 0 ? addressPosition[_address]: nftPosition[_poolId][_tokenId];

	(uint256 rewardsSinceLastCalculated,) = rewardsBy(_poolId, pool.lastRewardedTimestampHour, getPreviousTimestampHour());
	uint256 accumulatedRewardsPerShare = pool.accumulatedRewardsPerShare;

	if (block.timestamp > pool.lastRewardedTimestampHour + SECONDS_PER_HOUR && pool.stakedAmount != 0) {
		accumulatedRewardsPerShare = accumulatedRewardsPerShare + rewardsSinceLastCalculated * APE_COIN_PRECISION / pool.stakedAmount;
	}
	return ((position.stakedAmount * accumulatedRewardsPerShare).toInt256() - position.rewardsDebt).toUint256() / APE_COIN_PRECISION;
}

Function return too many parameters and the developer needs to worry about the order of the parameter.

line of code.

https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol#L74

vulnerability detail and recommended fix

the function calculateUserAccountData return too many parameters, the developer has to calibrate the order of the parameter, which is very error-prone.

(, , , , , , , uint256 healthFactor, , ) = GenericLogic
	.calculateUserAccountData(ps._reserves, ps._reservesList, params);
return healthFactor;

We recommend the project wrap the returned parameter in a struct so the function can access a parameter using struct.xxx, instead of figuring the order of the parameter.

outdated soliditiy version

line of code.

All file

vulnerability detail and recommended fix

The code use

// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.10;

We recommend the use the latest version (0.8.17) to make sure the bugs in the oudated version does not affect the implementation.

onERC721Received Hook is missing PoolMarketPlace.sol

line of code.

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/protocol/pool/PoolMarketplace.sol

vulnerability detail and recommended fix

We need to trace the function call in PoolMarketPlace.sol

/// @inheritdoc IPoolMarketplace
function batchAcceptBidWithCredit(
	bytes32[] calldata marketplaceIds,
	bytes[] calldata payloads,
	DataTypes.Credit[] calldata credits,
	address onBehalfOf,
	uint16 referralCode
) external virtual override nonReentrant {
	DataTypes.PoolStorage storage ps = poolStorage();

	MarketplaceLogic.executeBatchAcceptBidWithCredit(
		marketplaceIds,
		payloads,
		credits,
		onBehalfOf,
		ps,
		ADDRESSES_PROVIDER,
		referralCode
	);
}

which calls:

function executeBatchAcceptBidWithCredit(
	bytes32[] calldata marketplaceIds,
	bytes[] calldata payloads,
	DataTypes.Credit[] calldata credits,
	address onBehalfOf,
	DataTypes.PoolStorage storage ps,
	IPoolAddressesProvider poolAddressProvider,
	uint16 referralCode
) external {

which calls:

_acceptBidWithCredit(
	ps._reserves,
	ps._reservesList,
	ps._usersConfig[vars.orderInfo.maker],
	DataTypes.ExecuteMarketplaceParams({
		marketplaceId: vars.marketplaceId,
		payload: vars.payload,
		credit: credit,
		ethLeft: 0,
		marketplace: vars.marketplace,
		orderInfo: vars.orderInfo,
		weth: vars.weth,
		referralCode: referralCode,
		reservesCount: ps._reservesCount,
		oracle: poolAddressProvider.getPriceOracle(),
		priceOracleSentinel: poolAddressProvider
			.getPriceOracleSentinel()
	})
);

which calls:

_borrowTo(reservesData, params, vars, params.orderInfo.maker);

// delegateCall to avoid extra token transfer
Address.functionDelegateCall(
	params.marketplace.adapter,
	abi.encodeWithSelector(
		IMarketplace.matchBidWithTakerAsk.selector,
		params.marketplace.marketplace,
		params.payload
	)
);

_repay(
	reservesData,
	reservesList,
	userConfig,
	params,
	vars,
	params.orderInfo.maker
);

which calls:

_repay(
	reservesData,
	reservesList,
	userConfig,
	params,
	vars,
	params.orderInfo.maker
);

note the logic in the _repay function:

 // item.token == underlyingAsset but supplied after listing/offering
// so NToken is transferred instead
if (INToken(vars.xTokenAddress).ownerOf(tokenId) == address(this)) {
	_transferAndCollateralize(
		reservesData,
		userConfig,
		vars,
		token,
		tokenId,
		onBehalfOf
	);
	// item.token == underlyingAsset and underlyingAsset stays in wallet
} else {
	DataTypes.ERC721SupplyParams[]
		memory tokenData = new DataTypes.ERC721SupplyParams[](1);
	tokenData[0] = DataTypes.ERC721SupplyParams(tokenId, true);
	SupplyLogic.executeSupplyERC721(
		reservesData,
		userConfig,
		DataTypes.ExecuteSupplyERC721Params({
			asset: token,
			tokenData: tokenData,
			onBehalfOf: onBehalfOf,
			payer: address(this),
		   referralCode: params.referralCode
		})
	);
}

note the section:

DataTypes.ERC721SupplyParams[]
		memory tokenData = new DataTypes.ERC721SupplyParams[](1);
	tokenData[0] = DataTypes.ERC721SupplyParams(tokenId, true);
	SupplyLogic.executeSupplyERC721(
		reservesData,
		userConfig,
		DataTypes.ExecuteSupplyERC721Params({
			asset: token,
			tokenData: tokenData,
			onBehalfOf: onBehalfOf,
			payer: address(this),
		   referralCode: params.referralCode
		})
	);

the code assumes that the PoolMarketPlace hold NFT because payer is address(this), which is the PoolMarketPlace.sol

the problem is when trading, the Opensea NFT transfer does not use safeTransfer and does not require smart contract receiver to implement onERC721Received.

LooksRare NFT transfer does not use safeTransfer and does not require smart contract receiver to implement onERC721Received.

But X2Y2 market use safeTransfer actually. so without implement onERC721Received in PoolMarketPlace.sol, the PoolMarketPlace.sol contract is not capable of receiving NFT when the markplace use safeTransfer.

Thus we recommend the project add onERC721Received in the PoolMarketPlace.sol