sherlock-audit / 2023-01-ajna-judging

1 stars 0 forks source link

ctf_sec - CryptoKitty and CryptoFighter NFT can be paused, which block borrowing / repaying / liquidating action in the ERC721Pool when borrowers still forced to pay the compounding interest #34

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

ctf_sec

medium

CryptoKitty and CryptoFighter NFT can be paused, which block borrowing / repaying / liquidating action in the ERC721Pool when borrowers still forced to pay the compounding interest

Summary

CryptoKitty and CryptoFighter NFT can be paused, which block borrowing / repaying / liquidating action in the ERC721Pool

Vulnerability Detail

In the current implementation in the factory contract and the pool contract, special logic is in-place to handle non-standard NFT such as crypto-kitty, crypto-figher or crypto punk.

In the factory contract:

NFTTypes nftType;
// CryptoPunks NFTs
if (collateral_ == 0xb47e3cd837dDF8e4c57F05d70Ab865de6e193BBB ) {
    nftType = NFTTypes.CRYPTOPUNKS;
}
// CryptoKitties and CryptoFighters NFTs
else if (collateral_ == 0x06012c8cf97BEaD5deAe237070F9587f8E7A266d || collateral_ ==  0x87d598064c736dd0C712D329aFCFAA0Ccc1921A1) {
    nftType = NFTTypes.CRYPTOKITTIES;
}
// All other NFTs that support the EIP721 standard
else {
    // Here 0x80ac58cd is the ERC721 interface Id
    // Neither a standard NFT nor a non-standard supported NFT(punk, kitty or fighter)
    try IERC165(collateral_).supportsInterface(0x80ac58cd) returns (bool supportsERC721Interface) {
        if (!supportsERC721Interface) revert NFTNotSupported();
    } catch {
        revert NFTNotSupported();
    }

    nftType = NFTTypes.STANDARD_ERC721;
}

And in ERC721Pool When handling ERC721 token transfer:

/**
 *  @notice Helper function for transferring multiple NFT tokens from msg.sender to pool.
 *  @notice Reverts in case token id is not supported by subset pool.
 *  @param  poolTokens_ Array in pool that tracks NFT ids (could be tracking NFTs pledged by borrower or NFTs added by a lender in a specific bucket).
 *  @param  tokenIds_   Array of NFT token ids to transfer from msg.sender to pool.
 */
function _transferFromSenderToPool(
    uint256[] storage poolTokens_,
    uint256[] calldata tokenIds_
) internal {
    bool subset   = _getArgUint256(SUBSET) != 0;
    uint8 nftType = _getArgUint8(NFT_TYPE);

    for (uint256 i = 0; i < tokenIds_.length;) {
        uint256 tokenId = tokenIds_[i];
        if (subset && !tokenIdsAllowed[tokenId]) revert OnlySubset();
        poolTokens_.push(tokenId);

        if (nftType == uint8(NFTTypes.STANDARD_ERC721)){
            _transferNFT(msg.sender, address(this), tokenId);
        }
        else if (nftType == uint8(NFTTypes.CRYPTOKITTIES)) {
            ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transferFrom(msg.sender ,address(this), tokenId);
        }
        else{
            ICryptoPunks(_getArgAddress(COLLATERAL_ADDRESS)).buyPunk(tokenId);
        }

        unchecked { ++i; }
    }
}

and

uint8 nftType = _getArgUint8(NFT_TYPE);

for (uint256 i = 0; i < amountToRemove_;) {
    uint256 tokenId = poolTokens_[--noOfNFTsInPool]; // start with transferring the last token added in bucket
    poolTokens_.pop();

    if (nftType == uint8(NFTTypes.STANDARD_ERC721)){
        _transferNFT(address(this), toAddress_, tokenId);
    }
    else if (nftType == uint8(NFTTypes.CRYPTOKITTIES)) {
        ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transfer(toAddress_, tokenId);
    }
    else {
        ICryptoPunks(_getArgAddress(COLLATERAL_ADDRESS)).transferPunk(toAddress_, tokenId);
    }

    tokensTransferred[i] = tokenId;

    unchecked { ++i; }
}

note if the NFT address is classified as either crypto kitties or crypto fighers, then the NFT type is classified as CryptoKitties, then transfer and transferFrom method is triggered.

if (nftType == uint8(NFTTypes.CRYPTOKITTIES)) {
            ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transferFrom(msg.sender ,address(this), tokenId);
        }

and

else if (nftType == uint8(NFTTypes.CRYPTOKITTIES)) {
    ICryptoKitties(_getArgAddress(COLLATERAL_ADDRESS)).transfer(toAddress_, tokenId);
}

However, in both crypto-kitty and in crypto-figher NFT, the transfer and transferFrom method can be paused.

In crypto-figher NFT:

https://etherscan.io/address/0x87d598064c736dd0C712D329aFCFAA0Ccc1921A1#code#L873

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    public
    whenNotPaused
{

In Crypto-kitty NFT:

https://etherscan.io/address/0x06012c8cf97BEaD5deAe237070F9587f8E7A266d#code#L615

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    external
    whenNotPaused
{

note the WhenNotPaused modifier.

Impact

If the transfer and transferFrom is paused in CryptoKitty and CryptoFighter NFT, the borrowing and repaying and liquidating action is blocked in ERC721Pool, the user cannot fully clear his debt and has to pay the compounding interest when the transfer is paused.

Code Snippet

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC721PoolFactory.sol#L58-L80

https://github.com/sherlock-audit/2023-01-ajna/blob/main/contracts/src/ERC721Pool.sol#L591-L623

Tool used

Manual Review

Recommendation

Interest should not be charged when external contract is paused to borrower when the external contract pause the transfer and transferFrom.

grandizzy commented 1 year ago

we're going to remove support for those NFTs and support only wrapped NFTs - will be fixed with the same fix as for #163 #140 #31