A malicious signer can exploite reentrancy while Borrowing
Summary
In the Borrow process, I found a possible reentrancy attack here in useOffer().
Vulnerability Detail
In the normal borrowing process, it calls useOffer() in BorrowHandler.sol. In useOffer() It mints NFT's using safeMint for signer . As we know that safeMint call onERC721Received() before minting NFT. malicious signer can write exploit in onERC721Received() to exploite reentrancy here . attacker can again call borrow() to borrow another loan . If this happens it will mess-ups order of position id and some other's id like loan id.
POC:
1) Bob borrow Money using borrow() in BorrowFacet.
2) borrow() will further call useCollateral() in BorrowHandler.sol which will further triggers useOffer().
3) In useOffer() after all the validations it will call safeMint to mint NFT for the signer.
4) Now safeMint will trigger onERC721Received() in signers contract which will again calls borrow() to borrow some tokens . may be attacker can call borrow() multiple times . To change the order of id's
Impact
This reentrancy messes with the loanId of the supply position as you saw in the loan as well, firstPositionId won't be correctly written Also the assumption that the ids are in order is broken.
SPYBOY
high
A malicious signer can exploite reentrancy while Borrowing
Summary
In the Borrow process, I found a possible reentrancy attack here in
useOffer().Vulnerability Detail
In the normal borrowing process, it calls
useOffer()in BorrowHandler.sol. InuseOffer()It mints NFT's using safeMint for signer . As we know that safeMint call onERC721Received() before minting NFT. malicious signer can write exploit inonERC721Received()to exploite reentrancy here . attacker can again callborrow()to borrow another loan . If this happens it will mess-ups order of position id and some other's id like loan id. POC: 1) Bob borrow Money usingborrow()in BorrowFacet. 2)borrow()will further calluseCollateral()in BorrowHandler.sol which will further triggersuseOffer(). 3) InuseOffer()after all the validations it will call safeMint to mint NFT for the signer.4) Now safeMint will trigger
onERC721Received()in signers contract which will again callsborrow()to borrow some tokens . may be attacker can callborrow()multiple times . To change the order of id'sImpact
This reentrancy messes with the loanId of the supply position as you saw in the loan as well, firstPositionId won't be correctly written Also the assumption that the ids are in order is broken.
Code Snippet
borrow() : https://github.com/kairos-loan/kairos-contracts/blob/ce49230ab5255662d287c4944229cf411725de3f/src/BorrowFacet.sol#L38-L44 useCollateral() : https://github.com/kairos-loan/kairos-contracts/blob/ce49230ab5255662d287c4944229cf411725de3f/src/BorrowLogic/BorrowHandlers.sol#L100-L126 useOffer() : https://github.com/kairos-loan/kairos-contracts/blob/ce49230ab5255662d287c4944229cf411725de3f/src/BorrowLogic/BorrowHandlers.sol#L28-L93 safeMint call : https://github.com/kairos-loan/kairos-contracts/blob/ce49230ab5255662d287c4944229cf411725de3f/src/BorrowLogic/BorrowHandlers.sol#L91
Tool used
Manual Review
Recommendation
Use reentrancy guards.
Duplicate of #39