AbraNFT contest - AuditsAreUS's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L294-L297 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L234-L236

Vulnerability details

Impact

There is a potential reentrancy bug they may exist between requestLoan() and removeCollateral() that allows a user to have requested a loan while maintaining ownership of the collateral.

This bug is present in both NFTPair and NFTPairWithOracle.

The reentrancy attack requires the user to gain control of execution during beforeTokenTransfer() on the collateral token. While this is rare that developers relinquish control during this step it should be accounted for.

Proof of Concept

The attack works by first requesting a loan which transfers the collateral tokenId to the protocol.

The attacker is then able to call removeCollateral() which will delete the loan then begin to transfer the token. If the attacker is able to gain control during beforeTokenTransfer() the owner will still be NFTPair / NFTPairWithOracle.

    function removeCollateral(uint256 tokenId, address to) public {
        ...
        delete tokenLoan[tokenId];
        collateral.transferFrom(address(this), to, tokenId);
        emit LogRemoveCollateral(tokenId, to);
    }

Since the owner of the contract is still this it is possible to reenter the contract and requestLoan() with skim = true. We will pass the required checks and request a new loan.

    function _requestLoan(
        ...
        require(tokenLoan[tokenId].status == LOAN_INITIAL, "NFTPair: loan exists");
        if (skim) {
            require(collateral.ownerOf(tokenId) == address(this), "NFTPair: skim failed");

As the reentrancy completes the original removeCollateral() will finish executing collateral.transferFrom(address(this), to, tokenId); and the ERC712 token will be transferred to the attacker while the loan will still exist.

Recommended Mitigation Steps

There are two mitigations for this issue.

The first is to remove the skim functionality and instead forces the contract to transfer the token from the user to the NFTPair / NFTPairWithOracle.

The second mitigation is to add a reentrancy guard to each of the following functions:

• requestLoan()
• removeCollateral()
• requestAndBorrow()
• takeCollateralAndLend()
• repay()
• lend()

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L192-L196 https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L175-L179

Vulnerability details

Impact

The function init() in both NFTPair and NFTPairWithOracle are payable but do not use msg.value.

The impact is that any native currency sent in the init() function will sit in the contract. These tokens may be claimed by the first user to call cook() with ACTION_CALL to transfer the value to an attacker controlled address.

Proof of Concept

    function init(bytes calldata data) public payable override {
        require(address(collateral) == address(0), "NFTPair: already initialized");
        (collateral, asset) = abi.decode(data, (IERC721, IERC20));
        require(address(collateral) != address(0), "NFTPair: bad pair");
    }

Recommended Mitigation Steps

Consider enforcing msg.value == 0 in the init() function.