ParaSpace contest - 0xAgro's results

QA Report

Finding Summary

[NC-01] Inconsistent Leading Space In Comments

Most comments in the codebase start with a leading space. There are a few comments that void the general styling by ignoring a leading space. It is best to maintain consistant styling.

Findings:

/paraspace-core/contracts/misc/NFTFloorOracle.sol Links: 8, 10, 11, 13, 404, 408, 412.

8:	//we need to deploy 3 oracles at least
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
13:	//reject when price increase/decrease 1.5 times more than original value
404:	//first time just use the feeding value
408:	//use memory here so allocate with maximum length
412:	//aggeregate with price from all feeders

/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol Links: 314.

314:	vars.userGlobalDebt, //in base currency

/paraspace-core/contracts/protocol/libraries/logic/SupplyLogic.sol Links: 140, 141, 344, 345, 405, 406.

140:	//currently don't need to update state for erc721
141:	//reserve.updateState(reserveCache);
344:	//currently don't need to update state for erc721
345:	//reserve.updateState(reserveCache);
405:	//currently don't need to update state for erc721
406:	//reserve.updateState(reserveCache);

/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol Links: 353, 573, 685, 794.

353:	//add the current already borrowed amount to the amount requested to calculate the total collateral needed.
573:	//if collateral isn't enabled as collateral by user, it cannot be liquidated

/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol Links: 155, 171, 214, 304.

155:	//only partially withdraw need user's BAKC
171:	////transfer BAKC back for user
214:	//transfer BAKC back for user
304:	//transfer BAKC back for user

/paraspace-core/contracts/protocol/tokenization/base/MintableIncentivizedERC721.sol Links: 258.

258:	//solhint-disable-next-line max-line-length

/paraspace-core/contracts/protocol/tokenization/libraries/ApeStakingLogic.sol Links: 101, 103, 119, 161, 176.

101:	//1 unstake all position
103:	//1.1 unstake Main pool position
119:	//1.2 unstake bakc pool position
161:	//2 send incentive to caller
176:	//3 repay and supply

[NC-02] Inconsistent Capitalization In Comments

Most comments in the codebase start with a non-capital letter. There are a few comments that void the general styling by ignoring starting capitalized. It is best to maintain consistant styling.

Findings:

/paraspace-core/contracts/misc/NFTFloorOracle.sol Links: 17, 19.

17:	// Expiration Period for each feed price
19:	// Maximum deviation allowed between two consecutive oracle prices

/paraspace-core/contracts/misc/ParaSpaceOracle.sol Links: 24.

24:	// Map of asset price sources (asset => priceSource)

/paraspace-core/contracts/protocol/configuration/PoolAddressesProvider.sol Links: 21, 24, 27, 30.

21:	// Identifier of the ParaSpace Market
24:	// Map of registered addresses (identifier => registeredAddress)
27:	// Map of marketplace contracts (id => address)
30:	// Main identifiers

/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol Links: 66, 145, 232, 245, 385, 395. 450, 530, 536, 539, 549.

66:	// See `IPool` for descriptions
145:	// Auction related
232:	// If the collateral being liquidated is equal to the user balance,
245:	// Transfer fee to treasury if it is non-zero
385:	// If the collateral being liquidated is equal to the user balance,
395:	// Transfer fee to treasury if it is non-zero
450:	// Burn the equivalent amount of xToken, sending the underlying to the liquidator
530:	// Transfers the debt asset being repaid to the xToken, where the liquidity is kept
536:	// Handle payment
539:	// Burn borrower's debt token
549:	// Update borrow & supply rate

/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol Links: 189, 446.

189:	// Once we encounter a listing using WETH, then we convert all our ethLeft to WETH
446:	// No re-entrancy because it sent to our contract address

/paraspace-core/contracts/protocol/libraries/logic/PoolLogic.sol Links: 29.

29:	// See `IPool` for descriptions

/paraspace-core/contracts/protocol/libraries/logic/SupplyLogic.sol Links: 34.

34:	// See `IPool` for descriptions

/paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol Links: 43, 44, 47, 48.

43:	// Factor to apply to "only-variable-debt" liquidity rate to get threshold for rebalancing, expressed in bps
44:	// A value of 0.9e4 results in 90%
47:	// Minimum health factor allowed under any circumstance
48:	// A value of 0.95e18 results in 0.95

/paraspace-core/contracts/protocol/pool/PoolCore.sol Links: 168, 642.

168:	// Need to accommodate ERC721 and ERC1155 here
642:	// Reduces the length of the reserves array by `droppedReservesCount`

/paraspace-core/contracts/protocol/pool/PoolCore.sol Links: 792.

792:	// This function is necessary when receive erc721 from looksrare

/paraspace-core/contracts/protocol/tokenization/NToken.sol Links: 221.

221:	// Intentionally left blank

/paraspace-core/contracts/protocol/tokenization/NTokenMoonBirds.sol Links: 33.

33:	// Intentionally left blank

/paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol Links: 19, 21, 23, 25, 27, 29, 31, 33. 35, 37, 39 520, 526, 534, 549, 555, 563.

19:	// Token name
21:	// Token symbol
23:	// Mapping from token ID to owner address
25:	// Mapping from owner to list of owned token IDs
27:	// Mapping from token ID to index of the owner tokens list
29:	// Array with all token ids, used for enumeration
31:	// Mapping from token id to position in the allTokens array
33:	// Map of users address and their state data (userAddress => userStateData)
35:	// Mapping from token ID to approved address
37:	// Mapping from owner to operator approvals
39:	// Map of allowances (delegator => delegatee => allowanceAmount)
520:	// To prevent a gap in from's tokens array, we store the last token in the index of the token to delete, and
526:	// When the token to delete is the last token, the swap operation is unnecessary
534:	// This also deletes the contents at the last position of the array
549:	// To prevent a gap in the tokens array, we store the last token in the index of the token to delete, and
555:	// When the token to delete is the last token, the swap operation is unnecessary. However, since this occurs so
563:	// This also deletes the contents at the last position of the array

[NC-03] Long Lines (> 120 Characters)

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

Findings:

/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol Links: 11.

11:	import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

paraspace-core/contracts/misc/marketplaces/SeaportAdapter.sol Links: 11.

11:	import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

paraspace-core/contracts/misc/marketplaces/X2Y2Adapter.sol Links: 12.

12:	import {AdvancedOrder, CriteriaResolver, Fulfillment, OfferItem, ItemType} from "../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

paraspace-core/contracts/protocol/configuration/PoolAddressesProvider.sol Links: 7, 292, 338.

7:	import {InitializableImmutableAdminUpgradeabilityProxy} from "../libraries/paraspace-upgradeability/InitializableImmutableAdminUpgradeabilityProxy.sol";
292:	* @notice Internal function to update the implementation of a specific proxied component of the protocol that uses ParaProxy.
338:	* @dev It reverts if the registered address with the given id is not `InitializableImmutableAdminUpgradeabilityProxy`

paraspace-core/contracts/protocol/libraries/logic/AuctionLogic.sol Links: 94.

94:	* @notice Function to end auction on an ERC721 of a position if its ERC721 Health Factor increases back to above `AUCTION_RECOVERY_HEALTH_FACTOR`.

paraspace-core/contracts/protocol/libraries/logic/BorrowLogic.sol Links: 157.

157:	// if amount user is sending is less than payback amount (debt), update the payback amount to what the user is sending

paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol Links: 603, 655.

603:	* @return The actual debt that is getting liquidated. If liquidation amount passed in by the liquidator is greater then the total user debt, then use the user total debt as the actual debt getting liquidated. If the user total debt is greater than the liquidation amount getting passed in by the liquidator, then use the liquidation amount the user is passing in.
655:	* @return The maximum amount that is possible to liquidate given all the liquidation constraints (user balance, close factor)

paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol Links: 21.

21:	import {AdvancedOrder, CriteriaResolver, Fulfillment} from "../../../dependencies/seaport/contracts/lib/ConsiderationStructs.sol";

paraspace-core/contracts/protocol/libraries/logic/ValidationLogic.sol Links: 1137.

1137:	0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f, // keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")

paraspace-core/contracts/protocol/tokenization/NTokenApeStaking.sol Links: 100.

76:	* @notice Overrides the _transfer from NToken to withdraw all staked and pending rewards before transfer the asset
100:	* @notice Overrides the burn from NToken to withdraw all staked and pending rewards before burning the NToken on liquidation/withdraw

paraspace-core/contracts/protocol/tokenization/NTokenBAYC.sol Links: 48, 93, 95.

48:	* @notice Withdraw staked ApeCoin from the BAYC pool.  If withdraw is total staked amount, performs an automatic claim.
93:	* @notice Withdraw staked ApeCoin from the Pair pool.  If withdraw is total staked amount, performs an automatic claim.
95:	* @dev if pairs have split ownership and BAKC is attempting a withdraw, the withdraw must be for the total staked amount

paraspace-core/contracts/protocol/tokenization/libraries/MintableERC721Logic.sol Links: 530.

530:	erc721Data.ownedTokens[from][tokenIndex] = lastTokenId; // Move the last token to the slot of the to-delete token

paraspace-core/contracts/ui/WPunkGateway.sol Links: 71.

71:	* @dev supplies (deposits) WPunk into the reserve, using native Punk. A corresponding amount of the overlying asset (xTokens)

[NC-04] Order of Functions Not Compliant With Solidity Docs

The Solidity Style Guide suggests the following function ordering: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

Findings:

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

NFTFloorOracle.sol: public functions are positioned after external functions. ParaSpaceOracle.sol: public functions are positioned after internal functions. UniswapV3OracleWrapper.sol: external functions are positioned after public functions. PoolAddressesProvider.sol: external functions are positioned after public functions. GenericLogic.sol: internal functions are positioned after private functions. MarketplaceLogic.sol: external functions are positioned after internal functions. ValidationLogic.sol: internal functions are positioned after private functions. PoolApeStaking.sol: external functions are positioned after internal functions. PoolCore.sol: constructor is positioned after internal functions. PoolMarketplace.sol: external functions are positioned after internal fucntions. PoolParameters.sol: constructor is positioned after internal functions. MintableIncentivizedERC721.sol: external functions are positioned after public functions. NToken.sol: constructor is positioned after internal functions. NTokenApeStaking.sol: external functions are positioned after public functions. NTokenBAYC.sol: external functions are positioned after internal functions. NTokenMAYC.sol: external functions are positioned after internal functions. NTokenUniswapV3.sol: recieve function is positioned after external functions. MintableERC721Logic.sol: external functions are positioned after public functions.

[NC-05] File Layout Out of Order

The Solidity Style Guide suggests the following contract element ordering: Type declarations, State variables, Events, Modifiers, Functions.

Findings:

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

X2Y2Adapter.sol: struct is located after the constructor. NFTFloorOracle.sol: modifiers are located after functions, structs are after state variables. UniswapV3OracleWrapper.sol: structs are located after the constructor. LiquidationLogic.sol: structs are located after events. MarketplaceLogic.sol: structs are located after events. ValidationLogic.sol: struct located after function declarations. MintableIncentivizedERC721.sol: modifiers are located before state variables. ApeStakingLogic.sol: structs are located after state variables.

[NC-06] TODO Left In Production Code

TODOs should not be in production code. Each TODO should either be discarded or implemented if needed prior to production.

Findings:

/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol Links: 59.

59:	makerAsk.price, // TODO: take minPercentageToAsk into account

paraspace-core/contracts/misc/UniswapV3OracleWrapper.sol Links: 238.

238:	// TODO using bit shifting for the 2^96

paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol Links: 442.

442:	 // TODO: support PToken

[NC-07] Underscore Notation Not Used / Not Used Consistently

Consider using underscore notation to help with contract readability (Ex. 23453 -> 23_453).

Findings:

/paraspace-core/contracts/misc/NFTFloorOracle.sol Links: 12.

12:	uint128 constant EXPIRATION_PERIOD = 1800;

[NC-08] Out of Place And Inconsistant params Comment

Param indicators should be in a NatSpec comment above a function. The getBidOrderInfo has an out of place params comment inconsistant with all other functions in /paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol.

Findings:

/paraspace-core/contracts/misc/marketplaces/LooksRareAdapter.sol Links: 68

68:	bytes memory /*params*/

[NC-09] Empty Comment

Empty comments can be removed and replaced with a newline for more readable style.

Findings:

/paraspace-core/contracts/protocol/libraries/logic/MarketplaceLogic.sol Links: 202.

202:	//

[NC-10] Dead Comment Through Use of ////

There is no need to comment a quadruple /. //// can be changed to // to improve style or /// if a NatSpec comment.

Findings:

/paraspace-core/contracts/protocol/pool/PoolApeStaking.sol Links: 171.

171:	////transfer BAKC back for user

[NC-11] Comment Used Over NatSpec

Comment headers for functions should be in NatSpec format.

Findings:

L792 can be made into a @dev tag NatSpec comment.

/paraspace-core/contracts/protocol/pool/PoolCore.sol Links: 792.

792:	// This function is necessary when receive erc721 from looksrare