A time-boxed security review of the KS-PI2 protocol with a focus on the security aspects of the application's implementation.
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.
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 :
depositErc721
which will deposit the ERC721 token to the delegate.sol contract.approveDelegation
which will set the amount the borrower can borrow.For withdrawing the NFT, the debt needs to be equal to 0 when calling withdraw_NFT
.
The following method for borrowing :
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) :
userBorrowMai
- the caller will repay the entire debt of the owner of the vault.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
review commit hash - 79f6bfcce25854226711737576ffefad571ec127
The following smart contracts were in scope of the audit:
delegate
stableQiVault
The following number of issues were found, categorized by their severity:
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 |
Impact:
High, as the owner lost his vault
Likelihood:
High, just call to withdraw_NFT will be sufficient
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.
Add the same check as L87 at L131
+if(owner==false)
+ revert("You must own the vault to withdraw");
Impact:
High, the protocol can be DOS
Likelihood:
Low, owner trust the borrower
Here is the following logical errors :
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 :
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.
There is no need to import stableQiVault instead of using multiples interface because we are dealing with external contract
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;
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%.
- 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);
An optimization that can be done is to use memory variable inside functions instead of always calling storage.
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) {