AbraNFT contest - Czar102's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L205-L227

Vulnerability details

Impact

In NFTPair.sol#218 an 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 collateral contract, the severity is reduced to medium.

Proof of Concept

The exploit can be done in two steps:

1. 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 _requestLoan loan.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).
2. 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.