Gondi security record

Users can bundle up NFT and tokens in a special NFT called vaults via the `UserVault` contract. Tokens and NFTs bundled in such a vault are only accessible once the vault is burned again. The idea is that these vaults containing a set of NFTs and tokens can be used as collateral, as they give the owner access to the underlying assets once it is burned. The contract provides multiple functions to withdraw the different assets. While `withdrawTokens()` and `_withdrawEth()` work as expected, the `_withdrawNfts()` function does not check whether the vault held possession of the to-be-withdrawn NFT. As the minting and burning of vaults are done permissionlessly, anyone can drain all NFTs the contract holds.

In partial and full refinances, refinanced sources continue to receive a portion of the difference between the APR they originally offered and the APR the currently refinancing source offers. This concept is called proceeds and theoretically continues to accrue interest until a loan is repaid or renegotiated. This incentivizes early lenders to accept loans, as they might receive a passive income stream in return even if they get immediately refinanced. These proceeds are tracked in a separate dynamically sized array as part of the `Loan` struct. It is traversed and possibly appended to in nearly all interactions with a loan. It can become fully discarded only via renegotiations, where all proceeds are paid off by the borrower. In case of liquidations, the proceeds are not considered. The proceeds are only paid out in case of renegotiations and loan repayment. The array is appended to for each source that is being refinanced, so `_loan.sources.length` elements per transaction. It should be noted that the maximum number of sources a loan can be partitioned in is capped by `MultiSourceLoan._maxSources`, which according to the documentation will be set to ten. While in most cases, only a full refinance would possibly extend the array by e.g. ten, leaving follow-up refinances to only extend the array by one, a malicious user could possibly fully refinance all sources not via `refinanceFull()` but via `refinancePartially()`, refinancing each of the e.g. all ten sources individually, creating ten new entries in the `proceeds` array, while the loan continues to have ten active sources. With each such refinance, the proceeds array would grow by ten elements. If a user attempts to repay or renegotiate such a loan, they would be forced to settle the interest accumulated by all proceeds. This would require an ERC20 transfer for each of the refinanced sources throughout a loan's lifetime (or until its last renegotiation). The gas costs of an ERC20 `transferFrom()` call differ by implementation, but can be approximated by 65000 gas. With a block gas limit of 30 million on mainnet, there is a rough theoretical capacity of 461 transfers that can be handled within one transaction, though the reality will be much lower than that, given all the associated gas costs of e.g. the `repayLoan()` function. While significant gas costs would be associated with performing such a bloating attack, it would force a borrower into liquidation.

The protocol enables the initialization and repayment of loans paired with callback data, which enables hooks to whitelisted contracts with provided calldata. With that, users can take a loan with the principal of the to-be-taken loan being used to buy the collateral in the first place ("buy now pay later"/"BNPL"). It can also be used to pair the selling of the NFT directly with its repayment. For such a request, the borrower does not necessarily need to be the `msg.sender`, but can also sign parts of the loan information instead, which is then verified on-chain. However, the callback data for the hooks remains unsigned, which is very problematic, as front-runners can replace the callback data. This is problematic in both cases of the hook usage: For loan repayments via selling `_repaymentData.callbackData` can be arbitrarily adjusted to possibly perform market sells of the NFT to repay the loan that return just enough funds to the borrower to cover the loan expenses. While the contract being interacted with still has to be whitelisted, any data regarding which function to invoke with what values and how much (w)ETH to transfer can be manipulated. While this is still a fairly constrained environment, where a certain contract has to be invoked and enough funds have to be made accessible to the borrower to repay the loan (assuming they currently do not have any available), it can leave the borrower at a significant loss. For example, assume a borrower took a 1 ETH loan with 1% APR against a 50 ETH floor price NFT and wants to repay the loan after a year by selling the NFT and get the rest of the profit. If they sign as part of the `LoanRepaymentData`, anyone can replace the `callbackData` in that struck to edit in a sale to an offer as low as 1.1 ETH to settle all necessary debt, leaving the borrower at a massive loss. * For BNPL interactions, where a loan is taken and from the lended principal the collateral is purchased via the `_handleAfterPrincipalTransferCallback()`, the `callbackData` too could be replaced in a way that could be unintended for the borrower. While the function seems to be designed for leveraged buys, where the loan is fully used with added funds from the borrower (see GON-7), it is still an unnecessary risk to leave the `callbackData` unsigned. While it should be noted that such an attack seems not possible for Seaport offer filling, as both parties need to have signed and thereby agreed upon an offer beforehand, but Seaport is just one of the planned integrations and the code is already present to seamlessly integrate more that possibly have looser constraints.

The `LoanOffer` struct used to publish a loan on-chain supports multiple flows where finally, both borrower and lender are required to commit to the `LoanOffer` for a loan to be initiated, either via a signature or by being the sender of the transaction. A borrower can sign the `ExecutionData` containing a loan they would like to request and either provide a specific lender able to carry out the loan request or leave that field unset to enable anyone to lend the principal. Once they have signed such an `ExecutionData`, it is impossible for the requesting borrower to cancel the request. The only way a borrower can prevent the loan request from being filled is by revoking the approval of the specified NFT or by waiting for the loan request to expire. This is asymmetrical to the capabilities of a lender, who can cancel certain loan offers of a certain `offerId` at any time via `cancelOffer()` or `cancelRenegotiationOffer()`.

The `Leverage` contract can be used for protocol interactions paired with callbacks to execute once principal from a loan is received (`Leverage.afterPrincipalTransfer()`) or the loan's underlying collateral is received (`Leverage.afterNFTTransfer()`). However, in both of these callbacks, funds could get permanently stuck in the contract, as the `afterPrincipalTransfer()` callback assumes that the combination of `msg.value` and the loan's principal will exactly cover the purchase attempted in the callback. In `afterNFTTransfer()` the returned amount is also specified in the callback data and might not be fully aligned with the received funds.

The `UserVault` contract provides both a `fallback()` and `receive()` function. All native tokens deposited via direct transfers would be forever stuck in the contract, as only funds received via `depositEth()` would ever be withdrawable.

There can be a DoS if the winner of the auction of an NFT collateral from a liquidation of a partial loan reverts the `onERC721Received()` callback, as it is distributed via the `safeTransferFrom()` as part of the liquidation process. A successful bidder of an NFT collateral resulting from the liquidation of a loan with multiple principal sources can deny the payout of the tokens by manipulating the `onERC721Received()` callback, as a transfer of the collateral to the bidder via `safeTransferFrom()` is attempted as part of the liquidation process. This would result in both the bid and the NFT being permanently frozen, so there is little incentive for a bidder to perform such an attack, but it would still stop lenders from receiving any liquidation proceeds.

A borrower can request a loan that any lender can take by specifying a `LoanOffer` with an unspecified `lender` field. If for some reason, a borrower would ever sign a collection loan offer request with an unspecified lender, yet `capacity > 0`, there is the potential for unexpected behavior if some conditions are met. In general, all loan requests with unspecified lenders about to be put on-chain via `emitLoan()` can be front-ran by other lenders to overtake the loan. The only requirement for such cases would be that the front-running lender replaces their address in the `LoanExecutionData.lender` field. There are more specific cases, depending on the `offer.capacity` field. In cases of `offer.capacity > 0`: * Other borrowers can mark the capacity of that loan as used under loan conditions they fully determine themselves, as long as the lender in the `LoanOffer` also remains unspecified and the `offerId` matches, since `_used[offer.lender][offer.offerId]` would be shared between the two separate loans. However, `capacity > 0` for borrower-signed loan requests seems rather unuseful, as the borrower is required to sign off a specific NFT to use, which could only be repeatedly used if a loan is repaid before the underlying loan offer expires. In case `offer.capacity == 0`: * Anyone can deny such a loan request by front-running the emitting of the loan with an arbitrary loan that also leaves the lender unspecified and matches the `offerId` of the other loan. As the two loans share the unspecified lender and `offerId` can be arbitrarily forged, it is marked as canceled in the `isOfferCancelled` mapping.

A lender can propose a loan offer on a collection, allowing for multiple loans to be withdrawn up to a designated `capacity`. `_validateExecutionData()` validates that a new loan has not exceeded the lender's capacity. However, it checks for whether withdrawing the full `offer.principalAmount` would exceed the capacity, rather than the `executionData.amount` (the actual value that the borrower will be lent). This would prevent the execution of the loan, despite the possibility of the `executionData.amount` remaining within the lender's capacity.

None of the deposit functions validate that the provided vault exists. The NatSpec comment above `depositNft()` states that the Gondi team does not check that the vault exists and it is the responsibility of the caller to make sure of that. However, the DeFi user experience is still very poor and is therefore error-prone, especially for the vast majority of (new) users. Not validating that the vault exists could lead to users being unable to withdraw their NFTs, ERC20s, and ETH since they would have to first burn the non-existent vault ID which would revert.