Skim Accidental Transfer of Collateral

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L247-L268 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L267-L297

Vulnerability details

Issue: removeCollateral function does not perform any address checks if the loan status = LOAN_INITIAL.

Consequences: If anyone sends an NFT to the contract by itself, for whatever reason, an attacker can snatch it with removeCollateral.

Proof of Concept

• Someone sends collateral to the contract by mistake, or preparing to call requestLoan
• Attacker listens to the mempool and immediately calls removeCollateral for that collateralID.
• The NFT is transferred to the attacker.

Mitigations

removeCollateral should add a check for LOAN_INITIAL condition. Example:-> if(tokenLoan[tokenId].status == LOAN_INITIAL) require(tokenLoan[tokenId].borrower != address(0), "").

[cryptolyndon]

Duplicate of #22

[0xean]

closing as invalid, see #22