Possible reentrancy which allows to write incorrect supplyPositionIndex

Summary

Due to reentrancy attack it's possible to has at least two Loan with the same supplyPositionIndex. And if the Kairo service provides market data (information about Loans) to another services these consumers might receive wrong data and show incorrect price. So Attacker might manipulate market price.

Vulnerability Detail

1) Attacker has real NFT collection. 2) Attacker has poisoned ERC20 token, which named like USDC. 3) Attacker has NFT collection. 4) Attacker creates N Offers which want to buy NFT with fake token. 5) Attacker calls BorrowFacet.borrow with NFT and fake token. 6) In useCollateral function uint256 firstSupplyPositionId = supplyPositionStorage().totalSupply + 1;, firstSupplyPositionId will be written with 1. 7) Next, dive in useOffer and calls collatState.assetLent.checkedTransferFrom(signer, collatState.from, arg.amount);. AssetLent is poisoned token. 8) Poisoned ERC20 token calls BorrowFacet.borrow with NFT from step 3. 9) Then, NFTs will fill supplyPositionStorage().totalSupply with real Offers and now totalSupply = 2 (we used only one Offer). And Offer placed on tokenIdx = 1. 10) Poisoned ERC20 token increases supplyPositionStorage().totalSupply. totalSupply = 3, and poisoned Offer placed on tokenIdx = 2. 11) Borrowing finished. Now in storage placed two Loan: 11.1) The first one is real and it's loanId = 1, it's supplyPositionIndex = 1. 11.2) The second one is fake and it's loanId = 2, it's supplyPositionIndex = 1. 12) Next attacker might call external services with these two loans, and due to implementation detail these services might display prices in different ways. What can be in the hands of the attacker, depending on his goals.

Impact

It's possible to create two Loans with the same supplyPositionIndex, where:

The first Loan (loanId = 1) with NFT collection with one price, and supplyPositionIndex = 1.
The second loan will be filled with NFT collection with fake Token and another price, with another Loan (loanId = 2), and supplyPositionIndex = 1 will be the same.
The Kairo might provide information about prices to external services, and these services might be implemented in different ways. They might have bugs in implementation, and may not be able to distinguish between different NFTs with the same supplyPositionIndex.
Therefore, Attacker might manipulate the market, and provide incorrect market prices.

Code Snippet

1) https://github.com/kairos-loan/kairos-contracts/blob/b2fd98d62cf0f25ee1db2bd551cd7b4606a5a988/src/BorrowLogic/BorrowHandlers.sol#L160-L204

    function initializedLoan(
        CollateralState memory collatState,
        address from,
        NFToken memory nft,
        uint256 nbOfOffers,
        uint256 lent,
        uint256 firstSupplyPositionId
    ) internal view returns (Loan memory) {
...
 supplyPositionIndex: firstSupplyPositionId,
...
}

2) https://github.com/kairos-loan/kairos-contracts/blob/b2fd98d62cf0f25ee1db2bd551cd7b4606a5a988/src/BorrowLogic/BorrowHandlers.sol#L100-L126

function useCollateral(
        OfferArg[] memory args,
        address from,
        NFToken memory nft
    ) internal returns (Loan memory loan) {
       ...
        uint256 firstSupplyPositionId = supplyPositionStorage().totalSupply + 1;
        ...
        for (uint256 i = 0; i < nbOfOffers; i++) {
            collatState = useOffer(args[i], collatState);
            lent += args[i].amount;
        }
       ...
        loan = initializedLoan(collatState, from, nft, nbOfOffers, lent, firstSupplyPositionId);
        protocolStorage().loan[collatState.loanId] = loan;
        ...
    }

3) https://github.com/kairos-loan/kairos-contracts/blob/b2fd98d62cf0f25ee1db2bd551cd7b4606a5a988/src/BorrowLogic/BorrowHandlers.sol#L28-L93

   function useOffer(
        OfferArg memory arg,
        CollateralState memory collatState
    ) internal returns (CollateralState memory) {
        ...
// !!!! HERE WE CAN RE-ENTER IN THE BorrowHandlers once again with new data, but supplyPositionStorage().totalSupply will be the same !!!!
        collatState.assetLent.checkedTransferFrom(signer, collatState.from, arg.amount); 
        safeMint(signer, Provision({amount: arg.amount, share: shareMatched, loanId: collatState.loanId})); 
        return (collatState);
    }

Tool used

Manual Review

Recommendation

Add reentrancy guard for all functions.

Duplicate of #39

sherlock-audit / 2023-02-kairos-judging

glcanvas - Possible reentrancy which allows to write incorrect supplyPositionIndex #140