Aconomy Marketplace security record

The contract owner is allowed to perform a refund of the validator’s stake at any time deemed necessary (except when the contract is paused). The validator’s staked tokens could be an arbitrary token, say DAI for example, but the owner is not constrained to refund DAI tokens. They can refund any amount of any ERC20 tokens, which could be worth nothing at all. This way, a malicious/compromised owner account could deny validators access to their staked amounts and keep the staked value stuck in the contract still as there is no central withdraw / sweep function in validatorStake.sol

AconomyPoolFee(), AconomyPiMarketFee(), AconomyNFTLendBorrowFee() are public view methods used to return the values of _AconomyPoolFee,_AconomyPiMarketFee,_AconomyNFTLendBorrowFee variables respectively.Solidity creates getter methods for every public variable. the contract currently has these above-mentioned functions defined for returning the values, which makes it redundant to have other public methods created by solidity for _AconomyPoolFee,_AconomyPiMarketFee,_AconomyNFTLendBorrowFee.

ERC20Address address type variable from StakeDetail struct is not getting used. The AttestationRegistryAddress variable is not used in any of the contracts.

• In validatorStake.sol, Counters.sol is getting imported on L8 and L10, and using directive is getting used for it on L19. No functionality from Counters.sol is getting used hence it's redundant. Consider removing the import on L8,L10 and its usage on L19. • Same in PoolRegistry.sol, Counters.sol is getting imported on L4 and it’s using directive is on L21. It is also redundant.In piNFT.sol, IERC20 and IERC721Receiver are redundant imports. Consider removing the unused imports. • IERC20Upgradeable, IERC721ReceiverUpgradeable, Ownable are unused imports in ValidatedNFT.sol and can be removed. • IERC20, ERC721, LibCollection are unused imports in CollectionMethods.sol and can be removed. • Solidity pragma versions greater than 0.8.0 have internal underflow and overflow protection. Arithmetic operations that would result in underflow and overflow would result in an internal panic. The SafeMath library helps to protect from such issues in earlier Solidity versions but now becomes redundant as used here. • In NFTLendingBorrowing.sol, Counters.sol is getting imported on L10 and using directive is getting used for it on L23. No functionality from Counters.sol is getting used hence it's redundant. Consider removing the import on L10 and its usage on L23. • ERC721, Ownable are unused imports in PiMarket.sol and can be removed. ERC721 is imported on L5 and Ownable is imported on L6 but they are unused throughout the codebase and can be removed. • In piNFTMethods the import "./CollectionMethods.sol"; on L12 is unused hence can be removed. In poolAddress.sol , the Counters.sol is imported but never used hence can be removed.

In CollectionMethods.sol, the modifier that checks if a caller is the NFT owner is unused. Although CollectionMethods is imported in the piMarket contract, piMarket.sol defines onlyOwnerOfToken separately, not using the existing function modifier in CollectionMethods.sol.

On L203, L204 addERC20() function has require checks, The first statement checks that approvedValidator[_collectionAddress][_tokenId] should not be address(0) and the second one checks that msg.sender should be equal to approvedValidator[_collectionAddress][_tokenId].Instead of checking for both the conditions only one condition can be checked that is require(msg.sender == approvedValidator[_collectionAddress][_tokenId]);.  So if msg.sender == approvedValidator[_collectionAddress][_tokenId] is true then approvedValidator[_collectionAddress][_tokenId] can't be a zero address.

While adding the validator with addValidator() and lazyAddValidator(), No check is added in the piNFTMethods for checking if the validator has staked or not. There's the possibility that the validators that haven’t been staked can be also added by users in piNFTMethods.

The variable bidStartTime in the TokenMeta struct is declared but never used. It currently takes up an extra portion of storage in every instance of the TokenMeta struct created and will never be filled.

In Fundingpool.sol, InstallmentRepaid() is unused hence can be removed.In poolAddress.sol, repaidAmounts() event is unused hence can be removed.

The initialize() function in the ValidatedNFT contract does not initialize ERC721Upgradeable by calling __ERC721_init(). The ERC721Upgradeable is inherited in ERC721URIStorageUpgradeable. The __ERC721_init() sets the contract name and symbol in __ERC721_init(). Not calling the initializer function leaves the NFT without a name and symbol which could affect metadata parsing for each ValidatedNFT minted, even though they will have all other information intact.