Backed Protocol contest - sorrynotsorry's results

QA Report (Low / Non-Critical)

• The team can consider making the contracts Pausable so in case of an unexpected event, the owner/governance can pause transfers. Library can be utilized for this.

• Even transferFrom of OpenZeppelin's IERC721.sol and Rari Capital's SafeTransferLib.sol will not fail, it's still recommended that require statements should be built after checking the success return value of these calls prior updating state variables. E.g.

(bool success, ) = IERC721(collateralContractAddress).transferFrom(msg.sender, address(this), collateralTokenId);
if (success){
    unchecked {
    id = _nonce++;
    }
}

• At NFTLoanFacilitator.sol _nonce = is declared as 1 but there is no method to bring it to 0 in the contracts. So it can start counting from 1 with the first NFT by declaring but not initiliazing, also saving gas at the deployment.

GAS OPTIMIZATIONS

• IERC721().safeTransferFrom of OpenZeppelin uses function overloading first with memory data, second without memory data. Instead, the team can consider to explicitly use second safeTransferFrom function in the library without memory data args to save gas by adding require statement require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved").

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) public virtual override {
  
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved");
        _safeTransfer(from, to, tokenId, _data);
    }

• transferOwnership requires function overloading as per OpenZeppelin's library. The team can consider to expicitly call the second function by adding require statement as require(newOwner != address(0), "Ownable: new owner is the zero address")

    function _transferOwnership(address newOwner) internal virtual {
        address oldOwner = _owner;
        _owner = newOwner;
        emit OwnershipTransferred(oldOwner, newOwner);
    }
}

• constant variables INTEREST_RATE_DECIMALS, SCALAR can be declared as immutable to save gas.

• At NFTLoanFacilitator.sol, function interestOwed is calling _interestOwed at the same contract. It is understandable to call inherited functions with inlined methods. But on the same contract, this pattern consumes unnecessary gas. If the logic of _interestOwed would be inlined with interestOwed , that would prevent function overloading (not inheriting) and save gas.

• Using both named returns and a return statement isn't necessary. Removing unused named return variables can reduce gas usage (MSTOREs/MLOADs). To save gas and improve code quality: the team can consider using only one of those. Below methods use both the return statement and a named returns:

For NFTLoanFacilitator.sol; function loanInfoStruct(uint256 loanId) external view override returns (Loan memory) function totalOwed(uint256 loanId) external view override returns (uint256) function interestOwed(uint256 loanId) external view override returns (uint256) function loanEndSeconds(uint256 loanId) external view override returns (uint256) function _interestOwed( uint256 loanAmount, uint256 lastAccumulatedTimestamp, uint256 perAnumInterestRate, uint256 accumulatedInterest ) internal view returns (uint256)

For NFTLoanTicket.sol; function tokenURI(uint256 tokenId) public view override returns (string memory)

For BorrowTicketSVGHelper.sol ; All methods in BorrowTicketSVGHelper.sol For LendTicketDescriptor.sol ; All methods in LendTicketDescriptor.sol For LendTicketSVGHelper.sol ; All methods in LendTicketSVGHelper.sol For NFTLoansTicketDescriptor.sol ; All methods in NFTLoansTicketDescriptor.sol For TicketTypeSpecificSVGHelper.sol ; function colorStyles(string memory primary, string memory secondary) internal pure returns (string memory) function addressStringToHSL(string memory account) private pure returns (string memory)

For HexStrings.sol ; All methods in HexStrings.sol For NFTLoanTicketSVG.sol ; All methods in NFTLoanTicketSVG.sol For PopulateSVGParams.sol : All methods in PopulateSVGParams.sol

• The team can consider using bytes32 instead of string wherever possible to save gas since String is a dynamic data structure and therefore is more gas-consuming then bytes32. E.g. in NFTLoanTicketSVG.sol struct SVGParams {}

• Using custom errors instead of returning strings saves gas.