AbraNFT contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L287 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L321

Vulnerability details

Impact

The return value bool success of oracle.get() calls is ignored. This could lead to stale data or incorrect prices due to oracle issues.

Proof of Concept

NFTPairWithOracle.sol#L287

Change to

(bool success, uint256 rate) = loanParams.oracle.get(address(this), tokenId);

require(success, "ORACLE: Failed"); // @audit-info add check for `success`
require(rate != 0, "ORACLE: Zero"); // @audit-info add check for `rate != 0`

NFTPairWithOracle.sol#L321

(bool success, uint256 rate) = params.oracle.get(address(this), tokenId);

require(success, "ORACLE: Failed"); // @audit-info add check for `success`
require(rate != 0, "ORACLE: Zero"); // @audit-info add check for `rate != 0`

Tools Used

Manual review

Recommended mitigation steps

Add check for bool success value of oracle.get().

QA Report

Table of Contents

• Non-Critical Findings
  • [NC-01] - Contract implementations should inherit their interface
  • [NC-02] - Consider using solidity version 0.8.x
  • [NC-03] - Missing NatSpec documentation for public function
  • [NC-04] - Spelling mistakes
• Low Risk
  • [L-01] - init functions can be frontrun
  • [L-02] - onERC721Received not implemented

Non-Critical Findings

[NC-01] - Contract implementations should inherit their interface

Description

It's best practice for a contract to inherit from it's interface. This improves the contract's clarity and makes sure the contract implementation complies with the defined interface.

Findings

NFTPair.sol#L59:

contract NFTPair is BoringOwnable, Domain, IMasterContract { // @audit-info add interface INFTPair
  ...
}

Recommended mitigation steps

Inherit from the missing interface or contract.

[NC-02] - Consider using solidity version 0.8.x

Description

Consider using Solidity version 0.8.13 instead of using solidity 0.6.12.

More recent versions of solidity have compiler optimizations, user defined types (this would be very useful in the concentratedLiquidityPools code), overriding interface functions, reading from immutables, among other things. This could help reading and writing safe and clean code.

Findings

NFTPairWithOracle.sol#L20
NFTPair.sol#L20

Recommended mitigation steps

Consider using Solidity version 0.8.13.

[NC-03] - Missing NatSpec documentation for public function

Description

Ensure that the code is well commented on both with NatSpec and inline comments for better readability and maintainability. The comments should accurately reflect what the corresponding code does.

Some functions and/or function parameters are missing NatSpec comments.

Findings

NFTPair.sol#L181

Public function updateLoanParams is missing NatSpec comment

NFTPair.sol#L360

Function parameter deadline is missing NatSpec comment

NFTPair.sol#L400

Function parameter deadline is missing NatSpec comment

NFTPair.sol#L505

Public function repay is missing NatSpec comment

Recommended mitigation steps

Add missing Natspec comments.

[NC-04] - Spelling mistakes

Description

Multiple spelling mistakes found.

Findings

NFTPair.sol#L233

/// @param skim True if the token has already been transfered

transferred

NFTPair.sol#L90

// Track assets we own. Used to allow skimming the excesss.

excess

NFTPair.sol#L114

// `calculateIntest`.

calculateInterest

NFTPair.sol#L320

/// @param skim True if the funds have been transfered to the contract

transferred

NFTPair.sol#L351

/// @param skimCollateral True if the collateral has already been transfered

transferred

NFTPair.sol#L389

/// @notice Take collateral from a pre-commited borrower and lend against it

committed

NFTPair.sol#L394

/// @param skimFunds True if the funds have been transfered to the contract

transferred

NFTPair.sol#L434

/// of the above inquality) fits in 128 bits, then the function is

inequality

Same spelling mistakes in NFTPairWithOracle.sol

Recommended mitigation steps

Fix spelling mistakes.

Low Risk

[L-01] - init functions can be frontrun

Description

The init function used to initialize important contract state can be called by anyone.

The attacker can initialize the contract before the legitimate deployer, hoping that the victim continues to use the same contract. In the best case for the victim, they notice it and have to redeploy their contract costing gas.

Findings

NFTPair.sol#L175
NFTPairWithOracle.sol#L192

Recommended mitigation steps

Use the constructor to initialize non-proxied contracts.

For initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify the transaction succeeded.

[L-02] - onERC721Received not implemented

Description

Contracts do not implement the onERC721Received function, which is considered a best practice to transfer ERC721 tokens from contracts to contracts. The absence of this function could prevent contracts from receiving ERC721 tokens from other contracts via safeTransferFrom.

Findings

NFTPair.sol
NFTPairWithOracle.sol

Recommended mitigation steps

Consider adding an implementation of the onERC721Received function.