NFTCall

CertiK16 Mar 2023Files named in the report

Audit

CertiK reviewed NFTCall on 16 Mar 2023: 20 findings, 0 critical; 19 marked fixed, 1 open.

No open critical findings

Open the report Open NFTCall’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings20

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	0	0
Medium	0	1
Low	1	18

MediumAble To Change Preference During Exercise WindowFixed

The exercise window of an option includes the `endTime`. The function `changePreference()` should only be able to be called after an option has ended, however, it makes the following check: ```solidity=462 require(block.timestamp >= uint256(nftStatus[tokenId].getEndTime()), Errors.CP_NFT_ON_MARKET_OR_UNABAILABLE); ``` This check allows the preferences to be changed if the `block.timestamp` equals the `endTime`. This allows the preferences to be changed and the call to be exercised in the same block. This scenario can cause confusion and have the user calling `changePreference()` spend gas unnecessarily.

Low`createPool()` Will Always Return Zero AddressFixed

In the function `createPool()`, the return variable `pool` is never assigned as there is a new temorary variable `pool` created in the following line of code: ```solidity=26 address pool = deploy(address(this), erc721token, oracle, premium); ```

Low`tokenURI` Returns NFT URIFixed

Both contracts `NToken` and `CallToken` have the function `tokenURI()` return the `tokenURI` of the NFT and not a unique URI to the tokens.

LowAmbiguous Or Missing Emitted Error CodeFixed

Inside of the function `openCall()` the following line is returned but is not emitted. ```solidity vars.errorCode ``` This can cause confusion if the error code is nonzero. - In `openCall()` this will revert with the error message `Errors.CP_CAN_NOT_OPEN_CALL`. This can make it hard to determine the source of the revert.

LowCannot Open Call If `premiumToOwner` Equals `miniFixed

The value of `minimumPremiumToOwner` should be the smallest premium that will be accepted, however, in the function `_previewOpenCall()` this will revert if the calculated `premiumToOwner` is equal to the `minimumPremiumToOwner`.

LowMissing Emit EventsFixed

When `createPool()` is called in the `CallFactory` contract it creates a new pool, but in addition creates new `NToken` and `CallToken` contracts. There is never an event emitted for the newly created `NToken` or `CallToken` contracts.

LowMissing Error MessagesFixed

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller. --- In addition there is an error message that may be misleading. In `exerciseCall()` there is the following check: ```solidity require(remainValue == 0, Errors.CP_DID_NOT_SEND_ENOUGH_ETH); ``` A user may send more than the strike price in the `msg.value` and this will revert with an error stating they did not send enough ETH, however they sent too much ETH.

LowMissing Zero Address ValidationFixed

The function `collectProtocol()` should check if the `recipient` is the zero address.

LowNFTs Can Be LockedFixed

If NFTs are accidentally sent directly to the `NToken` contract, then they will become locked and unable to be recovered. The contract implements the `onERC721Received()` function, so even if `safeTransferFrom()` is used a user will be able to send NFTs directly to the contract.

LowOracle Must Return Price With 18 DecimalsFixed

The `strikePrice` stored when opening a call is calculated using the oracles returned price. If this does not have 18 decimals, then in `exerciseCall` the `strikePrice` that is compared against the `msg.value` will not have 18 decimals.

Show 9 more

LowOut of Scope DependenciesFixed

The contract, `Premium` serves as the underlying entity to interact with the pricing of premiums on call options. However, the scope of the audit assumes the input `_premiumMesh` is functionally correct and treats it as a black box.

LowOverriding Functions Are UnnecessaryFixed

In the contract `NToken`, the functions `_beforeTokenTransfer()` and `supportsInterface()` override their respective functions from the `ERC721` contract. However, when overriding, they only call super on that function, which will simply call the function from the ERC721 contract.

LowPossible ReentrancyFixed

In the batch functions that mint or transfer ERC721 tokens, the `onERC721Received()` hook can be used for reentrancy.

LowPotential Reentrancy (Out-of-Order Events)Fixed

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker manipulates the following contract by re-entering, they could potentially throw off any protocol listening for these events. Additionally, `exerciseCall()` does not conform to check effect interaction patterns by burning before all state changes have been completed. ### `withdrawETH()` can send ETH to an external contract which can trigger a re-entry on fallback which can cause events to be out of order: ```solidity=191 _safeTransferETH(to, amount); emit WithdrawETH(_msgSender(), to, amount); emit BalanceChangedETH(user, _balanceOf[user]); ``` ### `withdraw()` and `exerciseCall()` externally calls `Ntoken.burn()` which can cause events to be out of order: ```solidity=45 function burn(address user, address receiverOfUnderlying, uint256 tokenId) public override onlyOwner{ _burn(tokenId); IERC721(nft).safeTransferFrom(address(this), receiverOfUnderlying, tokenId); emit Burn(user, receiverOfUnderlying, tokenId); } ``` ### `deposit()` and `depositWithPreference()` externally calls `Ntoken.mint()` which can cause events to be out of order: ```solidity=40 function mint(address user, uint256 tokenId) public override onlyOwner{ _safeMint(user, tokenId); emit Mint(user, tokenId); } ```

LowThird-Party DependenciesFixed

The contract is serving as the underlying entity to interact with third-party `Oracles` and `NFT's` The scope of the audit treats third-party entities as black boxes and assumes their functional correctness. However, in the real world, third parties can be compromised and this may lead to lost or stolen assets.

LowTyposFixed

In the contract, `CallPool` and `Errors`, the following typos were found: - `CP_NFT_ON_MARKET_OR_UNABAILABLE` should be corrected to `CP_NFT_ON_MARKET_OR_UNAVAILABLE`; - `CP_DID_NOT_SEND_ENOUGHT_ETH` should be corrected to `CP_DID_NOT_SEND_ENOUGH_ETH`. In the contract, `DataTypes`, the following typo was found: - bit 128-192: minimumStrikePrice should be corrected to bit 128-191: minimumStrikePrice. In the contract, `CallPool`, the following typos were found: - In the comments above `STRIKE_PRICE_SCALE`, it should use "greater" as opposed to "great"; - In the function `withdraw()`, the comment `// Burn NToken` should be moved to just before the `NToken` is burned; - In the function `exerciseCall()`, the comment `// Burn NToken and transfer underlying NFT` should be moved to just before the `NToken` is burned.

LowUnused EventFixed

```solidity=7 event Activate(address account); ``` - `Activate` is declared in `ICallPoolEvents` but never emitted. --- ```solidity=8 event Deactivate(address account); ``` - `Deactivate` is declared in `ICallPoolEvents` but never emitted. --- ```solidity=18 event DepositETH(address indexed user, address indexed receiver, uint256 amount); ``` - `DepositETH` is declared in `ICallPoolEvents` but never emitted.

LowUnused InterfaceFixed

```solidity=9 interface IPriceOracleGetter { ``` - `IPriceOracleGetter` is declared but never used.

LowUsage of Magic NumbersFixed

The `STRIKE_PRICE_SCALE` and `MAXIMUM_STRIKE_PRICE` are determine based on the decimals of the price returned by the oracle and the decimals of the strike price. The implementation assumes that the decimals of the price returned by the oracle is 18 and the decimals of the strike price is 9. In addition, the `MAXIMUM_STRIKE_PRICE` is the maximum `uint64` multiplied by the `STRIKE_PRICE_SCALE`.