ParaSpace contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

In NFTFloorOracle, the index of every feeder is stored in the mapping feederPositionMap:

function _addFeeder(address _feeder)
        internal
        onlyWhenFeederNotExisted(_feeder)
    {
        feeders.push(_feeder);
        feederPositionMap[_feeder].index = uint8(feeders.length - 1);
        feederPositionMap[_feeder].registered = true;

When a feeder is removed using _removeFeeder, the corresponding entry in the mapping is also deleted. However, the function _removeFeeder also swaps the feeder with the last entryy in the feeders array before popping the last element. Therefore, the index of the element that is at the last position changes (unless the last element is deleted). However, the index (within feederPositionMap) of the last entry is not updated and the next removal will therefore no longer remove the feeder from the feeders array (because of the check feeders[feederIndex] == _feeder). Therefore, the system will still use a feeder that should have been removed within _combine (because the function iterates over the whole feeders array).

Proof Of Concept

Let's say we have three feeders [A, B, C, D] (i.e. with indices 0, 1, 2, 3 in the feederPositionMap mapping) in feeders. An owner now calls removeFeeder(B). Afterwards, feeders is [A, C, D], but feederPositionMap[C] is still 2. When removeFeeder(C) is called, the feeder will not be removed and it will still be used. When removeFeeder(D) is called, the transaction will even revert because of an out of bounds access (index 3).

Recommended Mitigation Steps

Also update the index of the swapped entry.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c01a980e5d6e15b2993b912c3569ed8b5236ff33/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L326

Vulnerability details

Impact

MintableIncentivizedERC721.safeTransferFrom calls _safeTransfer, which then calls _transfer. This function in turn calls MintableERC721Logic.executeTransfer for the actual transfer. However, MintableERC721Logic.executeTransfer only executes the transfer and does not call onERC721Received when the recipient is a smart contract. Because of that, the function will also not be called when safeTransferFrom is used. This is a violation of EIP 721 which states:

    ///  (...) . When transfer is complete, this function
    ///  checks if `_to` is a smart contract (code size > 0). If so, it calls
    ///  `onERC721Received` on `_to` and throws if the return value is not
    ///  `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`.

Proof Of Concept

Let's say that a MintableIncentivizedERC721 NFT is transferred to a smart contract that does not want to receive NFTs (for instance, because it does not have functionality to recover them). This contract therefore implements an onERC721Received function that rejects the transfer. However, even when using safeTransferFrom, this will be ignored and the transfer will still succeed, although it should not according to EIP 721.

Recommended Mitigation Steps

Call onERC721Received when the recipient is a smart contract.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/154c657d927c66fc8a45a9a6df881ec937323be8/paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol#L74 https://github.com/code-423n4/2022-11-paraspace/blob/c01a980e5d6e15b2993b912c3569ed8b5236ff33/paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol#L56

Vulnerability details

Impact

The oracles UniswapV3OracleWrapper and ParaSpaceFallabackOracle directly query the Uniswap spot price / reserves to determine the asset price. In UniswapV3OracleWrapper, this is done in the following code:

(uint160 currentPrice, int24 currentTick, , , , , ) = pool.slot0();

Similarly, ParaSpaceFallbackOracle directly calls getReserves on the Uniswap pair and passes this value to getAmountOut (to calculate the asset price):

(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( //@audit Flashloan attack?
                10**decimals,
                tokenReserves,
                ethReserves
            );

Proof Of Concept

This can be exploited by an attacker to manipulate the asset price temporarily with a flash loan. In the context of ParaSpace, two kind of attacks are possible: 1.) Undercollaterized loans: Here, an attacker temporarily inflates the asset price with a flash loan to take out a loan that would not be allowed with the real asset price (because the health factor would be too low). If done properly, this can lead to situations where the loans are undercollaterized (if the real prices would be used). 2.) Liquidating positions: An attacker can also temporarily bring the price of an asset down to liquidate a position that is healthy (if the real price is used).

Recommended Mitigation Steps

Use a TWAP. See for instance https://medium.com/blockchain-development-notes/a-guide-on-uniswap-v3-twap-oracle-2aa74a4a97c5 for details.

Lines of code

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

Vulnerability details

Impact

ParaSpaceOracle.getAssetPrice uses the result of latestAnswer() directly. This function is deprecated (see for instance the comment on Etherscan) and it is strongly recommended to use latestRoundData() instead. The problem with the current usage in ParaSpace is that the age of the answer is not checked. The answer can be arbitrarily old, but it will still be used as the price of the asset. This can lead to wrong valuations and allow borrows that should not be allowed (because of the health factor).

Proof Of Concept

Let's say the price of an asset decreased signficantly (e.g., 50%) over multiple days, but the chainlink feed was not updated for some reason. ParaSpace will still use the old price, which opens up an arbitrage possibility (buying on an exchange cheaply, taking out a loan and never paying it back) that could ultimately kill the protocol.

Recommended Mitigation Steps

Use latestRoundData() which provides the timestamp. If the answer is too old, use the fallback oracle.

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/c01a980e5d6e15b2993b912c3569ed8b5236ff33/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol#L579

Vulnerability details

Impact

MintableIncentivizedERC721 intends to support EIP165. It inherits from IERC165 and implements a supportsInterface function. However, this function is not compliant with EIP 165, which states

Therefore the implementing contract will have a supportsInterface function that returns:

true when interfaceID is 0x01ffc9a7 (EIP165 interface)
false when interfaceID is 0xffffffff
true for any other interfaceID this contract implements
false for any other interfaceID

In contrast to the specification, the supportsInterface of MintableIncentivizedERC721 does not return true for the interface ID 0x01ffc9a7. Because of that, the EIP 165 functionality is currently almost useless. Because according to EIP 165, smart contracts should first always perform a contract.supportsInterface(0x01ffc9a7) call and only then check if the desired interfaces are supported.

Note that this also makes the tokens not compliant with EIP 721. According to EIP 721, "Every ERC-721 compliant contract must implement the ERC721 and ERC165 interfaces" and clients can therefore rely on this to detect ERC721 tokens.

Proof Of Concept

A NFT marketplace wants to support ERC721 tokens only and does not allow selling any other assets. A way to check this that according to EIP 721 & EIP 165 (in fact the only way to check this) is the following code:

require(contract.supportsInterface(0x01ffc9a7) && contract.supportsInterface(0x80ac58cd));

However, this code will fail with MintableIncentivizedERC721, meaning they cannot be sold.

Recommended Mitigation Steps

Additionally call super.supportsInterface(interfaceId).

Lines of code

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

Vulnerability details

Impact

According to a comment in ParaSpaceOracle, a fallback oracle should be used when Chainlink returns negative prices:

* - If the returned price by a Chainlink aggregator is <= 0, the call is forwarded to a fallback oracle

However, this is currently not the case. The answer is first cast to a uint (from an int):

price = uint256(source.latestAnswer());

The fallback is then only used if the casted answer is 0. However, if source.latestAnswer() was smaller than 0, price will be a huge number (because of the casting) that will be wrongly used.

Proof Of Concept

Let's say source.latestAnswer() = -1. Then, price will be equal to type(uint256).max (i.e., a huge number) and this wrong price will be used by the system, leading to completely wrong valuations.

Recommended Mitigation Steps

Check that the latest answer is <= 0 before casting, use the fallback oracle in that case.