Try   HackMD

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

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 at 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





- 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

+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) {
Last changed by 
 
Willboy
0
165
Published on HackMD