PRAISE - Malicious actor can steal collaterals from any bucket index because the mergeOrRemoveCollateral() function misses the ownerOf check on the NFTs

[sherlock-admin]

PRAISE

high

Malicious actor can steal collaterals from any bucket index because the mergeOrRemoveCollateral() function misses the ownerOf check on the NFTs

Summary

There should be ownerOf check on functions that transfer NFTs

Vulnerability Detail

There should be checks to ensure that the caller/ msg.sender is the owner of the NFTs to withdraw as collateral from the provided list of bucket indices in the mergeOrRemoveCollateral() function.

Since the mergeOrRemoveCollateral() function allows claiming NFT collateral across multiple buckets there should be a ownerOf check to ensure that NFTs being withdrawn belongs to the caller.

Impact

A Malicious actor can steal NFT collaterals from any bucket index.

Code Snippet

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/ERC721Pool.sol#L333

Tool used

Manual Review

Recommendation

you can add a check like this require(ownerOf(tokenId) == msg.sender, Errors.ACCESS);

[Love4codes]

Escalate for 10 USDC

Since the mergeOrRemoveCollateral() function allows claiming NFT collateral across multiple buckets there should be a ownerOf check to ensure that NFTs being withdrawn belongs to the caller.

Msg.sender could be an attacker or anyone and not the owner of the collateral NFTs in the bucketsTokenIds array.

Considering how collaterals are added in the ERC721Pool.addCollaterals() here

in the _transferFromSenderToPool(bucketTokenIds, tokenIds_);

  function _transferFromSenderToPool(
        uint256[] storage poolTokens_,
        uint256[] calldata tokenIds_
    ) internal {
        for (uint256 i = 0; i < tokenIds_.length;) {
            uint256 tokenId = tokenIds_[i];
            if (!tokenIdsAllowed(tokenId)) revert OnlySubset();
            poolTokens_.push(tokenId);

            _transferNFT(msg.sender, address(this), tokenId);

            unchecked { ++i; }
        }
    }

The tokenIds_ are pushed into the bucketTokenIds array when adding collaterals.

Now the bucketTokenIds array will contain Collateral NFTs of various users added as collateral via ERC721Pool.addCollaterals().

BUT in mergeOrRemoveCollateral() function, bucketTokenIds array containing the collateral NFTs of various Users, not just that of the msg.sender is passed as poolToken_ when calling _transferFromPoolToAddress() to transfer the collateral NFTs from pool to msg.sender. Therefore NFTs sent to msg.sender might not really be his own.

I'll give two possible vulnerability scenarios here:

SCENARIO 1:

-- Now lets say msg.sender added TypeA NFT as collaterals via ERC721Pool.addCollaterals().

-- And he then calls ERC721.mergeOrRemoveCollateral() to remove some of his collaterals, since the poolToken_ is bucketTokenIds array which contains different collateral NFTs of various users, he might receive TypeB NFT.

Now the value of TypeA might be more than TypeB NFT. So the user ends up with an NFT that has a lesser monetary value than the NFT he deposited as collateral, Which is a loss for him.

SCENARIO 2:

-- A malicious actor can call mergeOrRemoveCollateral() function and pass a number that is greater than the amount of NFTs he added as collateral via ERC721.addCollateral() i.e the figure he puts as uint256 noOfNFTsToRemove_, param will be greater than the number of NFTs he actually deposited as collateral.

To mitigate this:

ERC721 tokens have the ownerOf function to identify their owners.

 function ownerOf(uint256 _tokenId) external view returns (address);

You'll see that here

So to mitigate this issue you can add a check with the ownerOf function like i did below:

require(ownerOf(tokenId) == msg.sender, Errors.ACCESS); //@audit you can add a check like this 

i think this might also throw some light on this --https://0xvolodya.hashnode.dev/nft-attacks#heading-missing-ownerof-whenever-function-arguments-contain-arbitrary-nft-id

i also think this vulnerability also exists in removeCollateral() function

[sherlock-admin]

Escalate for 10 USDC

Since the mergeOrRemoveCollateral() function allows claiming NFT collateral across multiple buckets there should be a ownerOf check to ensure that NFTs being withdrawn belongs to the caller.

Msg.sender could be an attacker or anyone and not the owner of the collateral NFTs in the bucketsTokenIds array.

Considering how collaterals are added in the ERC721Pool.addCollaterals() here

in the _transferFromSenderToPool(bucketTokenIds, tokenIds_);

  function _transferFromSenderToPool(
        uint256[] storage poolTokens_,
        uint256[] calldata tokenIds_
    ) internal {
        for (uint256 i = 0; i < tokenIds_.length;) {
            uint256 tokenId = tokenIds_[i];
            if (!tokenIdsAllowed(tokenId)) revert OnlySubset();
            poolTokens_.push(tokenId);

            _transferNFT(msg.sender, address(this), tokenId);

            unchecked { ++i; }
        }
    }

The tokenIds_ are pushed into the bucketTokenIds array when adding collaterals.

Now the bucketTokenIds array will contain Collateral NFTs of various users added as collateral via ERC721Pool.addCollaterals().

BUT in mergeOrRemoveCollateral() function, bucketTokenIds array containing the collateral NFTs of various Users, not just that of the msg.sender is passed as poolToken_ when calling _transferFromPoolToAddress() to transfer the collateral NFTs from pool to msg.sender. Therefore NFTs sent to msg.sender might not really be his own.

I'll give to possible vulnerability scenarios here:

SCENARIO 1:

-- Now lets say msg.sender added TypeA NFT as collaterals via ERC721Pool.addCollaterals().

-- And he then calls ERC721.mergeOrRemoveCollateral() to remove some of his collaterals, since the poolToken_ is bucketTokenIds array which contains different collateral NFTs of various users, he might receive TypeB NFT.

Now the value of TypeA might be more than TypeB NFT. So the user ends up with an NFT that has a lesser monetary value than the NFT he deposited as collateral, Which is a loss for him.

SCENARIO 2:

-- A malicious actor can call mergeOrRemoveCollateral() function and pass a number that is greater than the amount of NFTs he added as collateral via ERC721.addCollateral() i.e the figure he puts as uint256 noOfNFTsToRemove_, param will be greater than the number of NFTs he actually deposited as collateral.

To mitigate this:

ERC721 tokens have the ownerOf function to identify their owners.

 function ownerOf(uint256 _tokenId) external view returns (address);

You'll see that here

So to mitigate this issue you can add a check with the ownerOf function like i did below:

require(ownerOf(tokenId) == msg.sender, Errors.ACCESS); //@audit you can add a check like this 

i think this might also throw some light on this --https://0xvolodya.hashnode.dev/nft-attacks#heading-missing-ownerof-whenever-function-arguments-contain-arbitrary-nft-id

i also think this vulnerability also exists in removeCollateral() function

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[dmitriia]

This looks invalid, as there is a check in LenderActions's _removeMaxCollateral() ensuring the caller can only reach own funds:

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L658-L664

        Lender storage lender = bucket.lenders[msg.sender];

        uint256 lenderLpBalance;

        if (bucket.bankruptcyTime < lender.depositTime) lenderLpBalance = lender.lps;
        // revert if no LP to redeem
        if (lenderLpBalance == 0) revert NoClaim();

[grandizzy]

The protocol has 2 functionalities wrt collateral: borrower can pledge collateral and draw debt or traders could add quote token or collateral in buckets and swap them. The functionality pointed out in issue is the swap one, so for example if someone add quote token at price 1000 and price goes down, a trader could add collateral at price 1000 and claim the quote tokens from bucket. same for collateral, one could add in bucket and take the quote tokens. Hence no need to retain / enforce for the owner of collateral.

[hrishibhat]

Result: Invalid Unique Considering this issue a non-issue based on the above comments from Lead Watson and Sponsor

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• Love4codes: rejected