Nemeos security record

*Note: This issue was disclosed by the Nemeos team before the start of the security review.* Each deployed pool is associated with a fixed NFT collection (external address) and a minimal deposit percentage. However, `Pool.buyNFT()` accepts `collectionAddress_` as a parameter, breaking that association. This could allow borrowers to buy NFTs from other arbitrary collections using the pool funds allocated for another collection.

The `refundLoan()` function currently updates the `loan.nextPaymentTime` by directly adding the `MAX_LOAN_REFUND_INTERVAL`, which introduces a discrepancy in loan terms. Specifically, taking the case of a loan requested for 45 days, this calculation method allows the borrower to accrue interest for only the initial 45 days but grants an extension to repay the principal amount up to 60 days. This discrepancy arises because the `MAX_LOAN_REFUND_INTERVAL` is statically added to the `nextPaymentTime` without considering the original remaining loan duration which should be 15 days.

When a borrower repays the full loan amount, the NFTWrapper token associated with that loan is burned, and the underlying NFT is sent to the original borrower that created the loan. However, this behavior can be unexpected, as the NFTWrapper mints ERC-721 tokens that can be transferred to other addresses. Other users can exchange tokens by NFTWrapper tokens, but in the end, they will not get the underlying NFT.

The `Pool.redeem()` and `Pool.withdraw()` functions in the contract allow for withdrawals on behalf of another user, creating a potential security risk under certain conditions: - The affected user is a contract unable to manage withdrawn funds effectively. - This flaw could let attackers withdraw assets from other users, enhancing their own ROI returns. - It enables manipulation of the `Pool`'s daily interest rate, as withdrawals prompt an update to this rate. The absence of the `_spendAllowance()` logic in `Pool.withdraw()`, a feature originally included in the `ERC4626Upgradeable` contract, is the fundamental cause of this vulnerability. This mechanism is essential for verifying and upholding the spending limits designated by users, thus blocking unauthorized withdrawals.

`Pool` implements a vesting system that assigns a time period to each depositor based on the interest rate voted at deposit time. This is checked at `_withdraw()`, a function overridden from the `ERC4626` implementation of OpenZeppelin. This is easily bypassed by transferring the ERC-20 shares of the vault to another address, as `_withdraw()` checks the vesting time of `owner`, an arbitrary address passed by the caller.

The `_createLoan()` function in the contract calculates loan interest based on the interest rate at the time of the NFT purchase, not affecting loans set prior to any rate changes. This design permits a potential exploit where users can manipulate the interest rate by depositing assets to vote for a lower rate, buying the NFT at this reduced rate, and then withdrawing their assets. This sequence can be executed in a single transaction by executing the following transaction in order: 1. `depositAndVote()` 2. `buyNFT()` 3. `withdraw()` or `redeem()` This is possible since the `depositAndVote()` function, allows setting the `dailyInterestRate_` to zero. Consequently, users can withdraw their deposited assets anytime, as `_updateVestingTime()` calculates the locking period by multiplying `dailyInterestRate_` and `vestingTimePerBasisPoint` which will be equal to zero. An interest rate of zero effectively nullifies the locking period, enabling the withdrawal of assets and potentially allowing an attacker, especially through flash loan attacks, to lower the pool's daily interest rate significantly.

The functions `Pool.redeem()`, `Pool.depositAndVote()`, and `Pool.withdraw()` lack re-entrancy protection. Re-entrancy, a prevalent vulnerability in smart contracts, enables attackers to re-invoke a contract's functions before it finalizes its state, potentially leading to unintended behaviors like fund drainage, state manipulation, or logical execution errors.

`NFTFilter.verifyLoanValidity()` verifies the signature passed by the borrower and returns false if it was not signed by the Nemeos off-chain oracle. While verifying the signature, it increases an internal mapping that keeps track of all users' nonces. This is an external function without any access control. Although a valid signature is needed, it does not revert if the signer is not the oracle. Even if it reverted, an attacker could listen for `Pool.buyNFT()` transactions, get the signature from the oracle, and just call `verifyLoanValidity()`. This will increase the nonce of the user, and when the legit transaction executes it will revert.

When a borrower is late in his loan payments, other users can start a liquidation process. Any address can call `DutchAuctionLiquidator.buy()` and buy the NFT at `currentPrice` (linearly decreasing from the starting loan price). `DutchAuctionLiquidator.buy()` validates that the ETH being paid is greater than this price (strict equality will not work here as the price changes with block timestamp). However, it does not refund the user buying the NFT for that difference (`msg.value - currentPrice`). This difference will be sent to the user being liquidated in the `Pool` contract (`Pool.refundFromLiquidation()`).

"Dead" code refers to code that is never executed and hence makes no impact on the final result of running a program. Dead code raises a concern, since either the code is unnecessary or the necessary code's results were ignored. Here are some snippets of dead code found in the codebase: 1. `DutchAuctionLiquidator` inherits from OpenZeppelin's `Initializable`, but it does not use the `initializer` modifier or other features of it. 1. `Pool.sol` imports the following files but may not use them: `IERC20.sol` 1. `PoolFactory.liquidator` is never used in `PoolFactory`