# Introduction

A time-boxed security review of the **KS-PI2** protocol with a focus on the security aspects of the application's implementation.

# Disclaimer

A smart contract security review can never verify the complete absence of vulnerabilities. This is a time, resource and expertise bound effort where I try to find as many vulnerabilities as possible. I can not guarantee 100% security after the review or even if the review will find any problems with your smart contracts.

# About **KS-PI2**

**KS-PI2** is a feature on top of mai finance. With this feature, a user who owned a vault can allow an amount of borrowed token to another address. In order to let the other address to borrow, the manger of the vault needs to deposit the ERC721 token inside the delegate.sol contract.

There is only one way for anyone to create a delegate borrow :

1. Calling `depositErc721` which will deposit the ERC721 token to the delegate.sol contract.
2. Calling `approveDelegation` which will set the amount the borrower can borrow.
Calling createAllowlist which will create an ERC1155 token that will need a whitelist access (via a Merkle tree) for a caller to mint it

For withdrawing the NFT, the debt needs to be equal to 0 when calling `withdraw_NFT`.

The following method for borrowing :

1. `userBorrowMai` - the caller can borrow the amount allowed by the owner of the vault.

The following method for repaying a debt (anyone can repay a debt) :
1. `userBorrowMai` - the caller will repay the entire debt of the owner of the vault.

# Threat Model

### Roles & Actors
- Protocol admin - can add vault contract of mai finance
- Original vault owner - can deposit/withdraw NFT vault. Can delegate borrow amount to multiples borrowers
- Whitelisted borrower - can borrow tokens from an already existing vault
- Everyone - can repay a loan

### Security Interview

# Severity classification

| Severity | Impact: High | Impact: Medium | Impact: Low |
| ---------------------- | ------------ | -------------- | ----------- |
| **Likelihood: High** | Critical | High | Medium |
| **Likelihood: Medium** | High | Medium | Low |
| **Likelihood: Low** | Medium | Low | Low |

**Impact** - the technical, economic and reputation damage of a successful attack

**Likelihood** - the chance that a particular vulnerability gets discovered and exploited

**Severity** - the overall criticality of the risk

# Security Assessment Summary

**_review commit hash_ - [79f6bfcce25854226711737576ffefad571ec127](https://github.com/NandyBa/KS-PI2/blob/mai-finance_clarifie_thomas/mai-finance/contracts/delegate.sol)**

### Scope

The following smart contracts were in scope of the audit:

- `delegate`
- `stableQiVault`

The following number of issues were found, categorized by their severity:

- Critical & High: 1 issues
- Medium: 1 issues
- Informational: 3 issues

---

# Findings Summary

| ID | Title | Severity |
| ------ | ---------------------------- | ------------- |
| [C-01] | Anyone can withdraw a NFT deposited | Critical |
| [M-01] | Any Medium Title Here | Medium |
| [I-01] | Might be optimized to use interfaces instead of the stableQiVault.sol | Informational |
| [I-02] | Use payBackToken instead of payBackTokenAll | Informational |
| [I-03] | Use memory variable instead of always calling storage | Informational |

# Detailed Findings

# [C-01] Anyone can withdraw a NFT deposited

## Severity

**Impact:**
High, as the owner lost his vault

**Likelihood:**
High, just call to withdraw_NFT will be sufficient

## Description
There is a check on whether the msg.sender is the owner of the vault or not. However, there is no revert if the assessment is not true.
Basically, if a vault has no debt then anyone can withdraw the NFT.
## Recommendations
Add the same check as [L87](https://github.com/NandyBa/KS-PI2/blob/mai-finance_clarifie_thomas/mai-finance/contracts/delegate.sol#L87) at [L131](https://github.com/NandyBa/KS-PI2/blob/mai-finance_clarifie_thomas/mai-finance/contracts/delegate.sol#L131)

+if(owner==false)
+   revert("You must own the vault to withdraw");

# [M-01] Logical errors on some variables

## Severity

**Impact:**
High, the protocol can be DOS

**Likelihood:**
Low, owner trust the borrower

## Description
Here is the following logical errors :
1. A malicious borrower can borrow more delegate amount that it supposed to be.
There is a chance that an owner delegate only 10% of the max borrow amount possible. In the current implementation, the borrower can call multiples times userBorrowMai until he reached the max amount borrowed possible. The problem is that the variables are not correctly updated. Instead of being increased, there will always be equal to the same value.
2. If the owner delegate to multiples borrowers, the variable vaultDebt [_vault][_erc721_Id] is not correctly updated
Indeed, if an owner delagate to multiple borrowers. The variable will allways be equal to the amount borrow by the last borrower. Instead of being increased.
3. If the owner wants to change the amount delegated, ...
No real recommendation

With 1. the borrower is not even able to repay his debt because there will be a difference between hasBorrowed[_owner][_borrower][_vault][_erc721_Id] and paybackTokenAll() (hasBorrowed[_owner][_borrower][_vault][_erc721_Id] < paybackTokenAll() ).At the end, the NFT will be stuck.

With 2. this is the same thing, but this time the problem comes from the possibility of multiple borrowers.

There will be always the following solution to repay a debt but it's not optimized :
- The user send this amount(totalBorrow - hasBorrowed[_owner][_borrower][_vault][_erc721_Id]) directly through the smart contract. Then the user call the function repayLoan()

## Recommendations
Globally, this should be review by the team. Depending on the possibility they want, there can be multiples recommendation.

In order to implement correctly, the fact that an owner can delegate to multiples borrowers. Or a malicious borrower can call multiples userBorrowMai if possible. We should modify at [L102](https://github.com/NandyBa/KS-PI2/blob/mai-finance_clarifie_thomas/mai-finance/contracts/delegate.sol#L102)

- hasBorrowed[_owner][_borrower][_vault][_erc721_Id] = _amount;
- vaultDebt[_vault][_erc721_Id] = _amount
+ hasBorrowed[_owner][_borrower][_vault][_erc721_Id] += _amount;
+ vaultDebt[_vault][_erc721_Id] += _amount

The [I-02] can also be a recommendation.

# [I-01] Might be optimized to use interfaces instead of the stableQiVault.sol

## Description
There is no need to import stableQiVault instead of using multiples interface because we are dealing with external contract

## Recommendations
Use these interface

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

pragma solidity ^0.8.0;


interface StableQiVault {
    function borrowToken(
        uint256 vaultID,
        uint256 amount,
        uint256 _front
    ) external;
    
    function paybackTokenAll(
        uint256 vaultID,
        uint256 deadline,
        uint256 _front
    ) external;

# [I-02] Use payBackToken instead of payBackTokenAll

## Description
In the current implementation, the caller of repayLoan needs to repay all the debt of the owner.
Might be better that the caller repay the amount that he wants.
We can image that the borrower can repay only 80% of the amount borrowed, then the original owner can repay the last 20%.
In the current implementation, the original owner will needs to repay 100%.

## Recommendations

- if(_amount*10 ** 18!=hasBorrowed[_owner][_borrower][_vault][_erc721_Id])
-            revert("You must repay the amount of the loan");
- _qiVault.paybackTokenAll(_erc721_Id, block.timestamp, 0);
+ _qiVault.paybackToken(_erc721_Id,_amount * (10 ** 18) , 0);

# [I-03] Use memory variable instead of always calling storage

## Description
An optimization that can be done is to use memory variable inside functions instead of always calling storage.
## Recommendations
Example at [L125](https://github.com/NandyBa/KS-PI2/blob/mai-finance_clarifie_thomas/mai-finance/contracts/delegate.sol#L125)

+uint length = isOwner[msg.sender][_vault].length - 1;
- for (uint i = 0; i < isOwner[msg.sender][_vault].length - 1; i++) {
+ for (uint i = 0; i < length; ++i) {