Foundation contest - robee's results

Title: Unsafe Cast Severity: Low/Medium Risk

use openzeppilin's safeCast in:

    FETH.transferFrom : unsafe cast uint96(amount)
    NFTMarketBuyPrice.setBuyPrice : unsafe cast uint96(price)
    FETH._freeFromEscrow : unsafe cast uint32(escrowIndex)
    FETH._marketUnlockFor : unsafe cast uint96(amount)
    FETH._deductBalanceFrom : unsafe cast uint96(amount)

Title: Treasury may be address(0) Severity: Low Risk

Make sure the treasury is not address(0).
    
    
    FNDNFTMarket.sol.constructor treasury
    FoundationTreasuryNode.sol.constructor _treasury

Title: Hardcoded WETH Severity: Low Risk

WETH address is hardcoded but it may differ on other chains, e.g. Polygon, so make sure to check this before deploying and update if necessary You should consider injecting WETH address via the constructor. (previous issue: https://github.com/code-423n4/2021-10-ambire-findings/issues/54) Hardcoded weth address in FETH.sol

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    AccessControlUpgradeable.sol

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    NFTMarketPrivateSale.sol, buyFromPrivateSale is missing a reentrancy modifier
    NFTMarketBuyPrice.sol, buy is missing a reentrancy modifier
    NFTMarketReserveAuction.sol, adminAccountMigration is missing a reentrancy modifier
    SendValueWithFallbackWithdraw.sol, withdraw is missing a reentrancy modifier
    NFTMarketReserveAuction.sol, updateReserveAuction is missing a reentrancy modifier
    NFTMarketReserveAuction.sol, placeBid is missing a reentrancy modifier

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    FoundationTreasury.sol, initialize, 61
    FNDNFTMarket.sol, initialize, 105

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    SendValueWithFallbackWithdraw.sol: function _sendValueWithFallbackWithdraw parameter gasLimit isn't used. (_sendValueWithFallbackWithdraw is internal)

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    AdminRole.sol, getAdminMemberCount
    NFTMarketCore.sol, getFethAddress
    NFTMarketBuyPrice.sol, getBuyPrice
    NFTMarketReserveAuction.sol, getReserveAuctionIdFor
    SendValueWithFallbackWithdraw.sol, getPendingWithdrawal

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    
    NFTMarketBuyPrice.sol._transferFromEscrowIfAvailable recipient
    NFTMarketReserveAuction.sol._transferFromEscrowIfAvailable nftContract
    FETH.sol.depositFor account

Title: transfer return value of a general ERC20 is ignored Severity: Low Risk

Need to use safeTransfer instead of transfer. The transfer return value has to be checked (as there are some other tokens that returns false instead revert).

    NFTMarketCore.sol, 105 (_transferToEscrow), IERC721(nftContract).transferFrom(msg.sender, address(this), tokenId);
    NFTMarketCore.sol, 86 (_transferFromEscrow), IERC721(nftContract).transferFrom(address(this), recipient, tokenId);
    NFTMarketPrivateSale.sol, 177 (buyFromPrivateSaleFor), nftContract.transferFrom(seller, msg.sender, tokenId);
    NFTMarketCore.sol, 97 (_transferFromEscrowIfAvailable), IERC721(nftContract).transferFrom(address(this), recipient, tokenId);

Title: Change transferFrom to transfer Severity: GAS

'transferFrom(address(this), , **)' could be replaced by the following more gas efficient 'transfer(, **)' This replacement is more gas efficient and improves the code quality.

    NFTMarketCore.sol, 86 : IERC721(nftContract).transferFrom(address(this), recipient, tokenId);
    NFTMarketCore.sol, 97 : IERC721(nftContract).transferFrom(address(this), recipient, tokenId);

Title: Unnecessary cast Severity: Gas

    address NFTMarketBuyPrice.sol._buy - unnecessary casting address(nftContract)

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    SendValueWithFallbackWithdraw.sol, __gap
    Constants.sol, READ_ONLY_GAS_LIMIT
    Constants.sol, MAX_ROYALTY_RECIPIENTS_INDEX
    Constants.sol, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS
    AccessControlUpgradeable.sol, __gap
    FoundationTreasuryNode.sol, __gap_was_treasury
    NFTMarketCore.sol, __gap
    NFTMarketPrivateSale.sol, __gap_was_DOMAIN_SEPARATOR
    NFTMarketAuction.sol, __gap
    NFTMarketReserveAuction.sol, __gap_was_config
    CollateralManagement.sol, __gap
    Constants.sol, SEND_VALUE_GAS_LIMIT_SINGLE_RECIPIENT
    NFTMarketFees.sol, __gap_was_fees
    AdminRole.sol, __gap
    Constants.sol, MIN_PERCENT_INCREMENT_IN_BASIS_POINTS
    NFTMarketBuyPrice.sol, __gap

Title: Unused declared local variables Severity: GAS

Unused local variables are gas consuming, since the initial value assignment costs gas. And are a bad code practice. Removing those variables will decrease the gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    ERC165Checker.sol, supportsERC165Interface, encodedParams

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    NFTMarketBuyPrice.sol, line 11, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
    NFTMarketFees.sol, line 12, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
    NFTMarketBuyPrice.sol, line 5, import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
    NFTMarketBuyPrice.sol, line 7, import "./Constants.sol";
    NFTMarketReserveAuction.sol, line 14, import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    LockedBalance.sol, 113: change 'balance > 0' to 'balance != 0'

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    FoundationTreasuryNode.sol, getFoundationTreasury

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    FETH.sol (L120), string public constant name = "Foundation Wrapped Ether"; 
    NFTMarketPrivateSale.sol (L38), string private constant NAME = "FNDNFTMarket"; 
    FETH.sol (L125), string public constant symbol = "FETH"; 

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    AccessControlUpgradeable.sol, grantRole
    AccessControlUpgradeable.sol, getRoleMemberCount
    AccessControlUpgradeable.sol, __AccessControl_init_unchained
    ERC165Checker.sol, supportsERC165
    ERC165Checker.sol, supportsERC165Interface
    LockedBalance.sol, setTotalAmount

Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    AccessControlUpgradeable.sol, getRoleAdmin, { return _roles[role].adminRole; }
    AccessControlUpgradeable.sol, getRoleMemberCount, { return _roles[role].members.length(); }
    AccessControlUpgradeable.sol, getRoleMember, { return _roles[role].members.at(index); }
    AccessControlUpgradeable.sol, hasRole, { return _roles[role].members.contains(account); }

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    AccountMigrationLibrary.requireAuthorizedAccountMigration (signature)
    NFTMarketReserveAuction.adminAccountMigration (signature)

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    FNDNFTMarket.sol.constructor

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    FETH.sol (L#528) - expiration = lockupDuration + block.timestamp.ceilDiv(lockupInterval) * lockupInterval;
    NFTMarketPrivateSale.sol (L#135) - } else if (deadline > block.timestamp + 2 days) { 
    NFTMarketReserveAuction.sol (L#427) - auction.endTime = block.timestamp + auction.duration;
    FETH.sol (L#718) - if (escrow.expiration >= block.timestamp && escrow.totalAmount > 0) { ++lockedCount;
    NFTMarketReserveAuction.sol (L#452) - auction.endTime = block.timestamp + auction.extensionDuration;

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    FNDNFTMarket.sol, _afterAuctionStarted
    AccessControlUpgradeable.sol, __AccessControl_init_unchained
    FNDNFTMarket.sol, _getSellerFor
    AccountMigrationLibrary.sol, _toAsciiString
    NFTMarketBuyPrice.sol, _afterAuctionStarted
    FNDNFTMarket.sol, _transferToEscrow
    NFTMarketBuyPrice.sol, _getSellerFor
    FNDNFTMarket.sol, _transferFromEscrowIfAvailable
    FNDNFTMarket.sol, _transferFromEscrow
    NFTMarketReserveAuction.sol, _getSellerFor

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    FNDNFTMarket.sol; the inherited contracts Constants, SendValueWithFallbackWithdraw, NFTMarketFees, NFTMarketReserveAuction, NFTMarketPrivateSale not used
    WithdrawFromEscrow.sol; the inherited contracts AdminRole not used
    FoundationTreasury.sol; the inherited contracts OperatorRole, CollateralManagement, WithdrawFromEscrow not used
    NFTMarketReserveAuction.sol; the inherited contracts FoundationTreasuryNode not used
    CollateralManagement.sol; the inherited contracts AdminRole not used