Anyone who gains execution during this transfer (after the owner of the token is changed) can steal the token transferred. Note that it will be applicable only if !skim.
Since the exploit makes assumptions about the implementation of the collateral contract, the severity is reduced to medium.
Proof of Concept
The exploit can be done in two steps:
Call requestLoan() with the same tokenId as the caller, any params, set to to the exploiting contract's address and skim to true. In the previous context of _requestLoanloan.status hasn't been set to LOAN_REQUESTED yet so the function will continue with execution. The collateral skim check will pass as the token has already been transferred to the contract. Thus the exploiter contract will temporarily set themselves as the borrower (because this will be undone when the primary requestLoan() finishes its execution).
Right after the requestLoan() call removeCollateral() to get the ERC-721 token. The attacker contract is temporarily the borrower as the primary requestLoan() hasn't written loan.borrower yet. So the ownership check will pass and collateral will be possible to be removed by an attacker.
Note that even if the owner of the token can gain execution, this exploit is still viable. Collateral transfer will not fail, creating a loan position with no collateral. This presents an equally serious threat, if not more, since a lender can agree to a proposition and lose their principal.
Tools Used
Manual analysis
Recommended Mitigation Steps
Write tokenLoan[tokenId].status = LOAN_REQUESTED before the collateral transfer.
cryptolyndon
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L205-L227
Vulnerability details
Impact
In
NFTPair.sol#218an ERC-721 transfer occurs.Anyone who gains execution during this transfer (after the owner of the token is changed) can steal the token transferred. Note that it will be applicable only if
!skim.Since the exploit makes assumptions about the implementation of the
collateralcontract, the severity is reduced to medium.Proof of Concept
The exploit can be done in two steps:
requestLoan()with the same tokenId as the caller, any params, settoto the exploiting contract's address andskimtotrue. In the previous context of_requestLoanloan.statushasn't been set toLOAN_REQUESTEDyet so the function will continue with execution. The collateral skim check will pass as the token has already been transferred to the contract. Thus the exploiter contract will temporarily set themselves as the borrower (because this will be undone when the primaryrequestLoan()finishes its execution).requestLoan()callremoveCollateral()to get the ERC-721 token. The attacker contract is temporarily the borrower as the primaryrequestLoan()hasn't writtenloan.borroweryet. So the ownership check will pass and collateral will be possible to be removed by an attacker.Note that even if the owner of the token can gain execution, this exploit is still viable. Collateral transfer will not fail, creating a loan position with no collateral. This presents an equally serious threat, if not more, since a lender can agree to a proposition and lose their principal.
Tools Used
Manual analysis
Recommended Mitigation Steps
Write
tokenLoan[tokenId].status = LOAN_REQUESTEDbefore the collateral transfer.