Fungify CERC721NoBorrow Security Review Report
February 2024
Overview
This audit covered a single commit for Fungify, a new lending protocol that builds on Compound to support lending/borrow of NFTs and introduces a special interest market token that is linked to NFT markets. This commit introduced a new type of token, namely CERC721NoBorrow, which is very similar to CERC721, except that deposited NFTs stay linked to the original owner and they can only be used as collateral and not borrowed. Our security assessment was a review of the smart contracts changes in this commit, spanning a total of 2 days During our audit, we did not identify any major vulnerabilities. We did identify 2 optimisations. Finally, all of our reported issues were fixed by the development team and consequently validated by us. We can confidently say that the overall security and code quality have increased after completion of our audit.
Scope
The analyzed resources are located on:
https://github.com/fungify-dao/taki-contracts/commit/197e76662fb13e977fe8ffd92782fb8b4fec7386
The issues described in this report were fixed in the following commit:
https://github.com/fungify-dao/taki-contracts/commit/e545376cf95c4844aaa4b00cf6f7a7336b8d188f
Summary
Weaknesses
This section contains the list of discovered weaknesses.
FNG-22. OPTIMISATION OF NFT TRANSFER OUT BY ID
Severity:
Status:
Fixed
Path:
CERC721NoBorrow.sol:doNFTTransferOutByIds:L179-214
Description:
CERC721NoBorrow implements a function doNFTTransferOutByIds to transfer NFTs out to the receiver using specific IDs.
In the function, it loops over each given NFT ID and the currently held NFTs of the user. Once found, the NFT is transferred to the receiver and the NFT is deleted from the userHeldNFTs array.
However, in the case where the given NFT ID is the last one in the array, it will do a redundant SSTORE of the NFT ID to the last element of the array on line 207 before popping the same value. This can be prevented by checking the index against the length.
function doNFTTransferOutByIds(address to, uint[] memory nftIds) internal {
uint[] storage userHeldNFTs_ = userHeldNFTs[to];
uint len = userHeldNFTs_.length;
uint nftCount = nftIds.length;
if (len < nftCount) {
revert NftAmountTooHigh();
}
IERC721 underlying_ = IERC721(underlying);
for (uint i = 0; i < nftCount;) {
uint nftID = nftIds[i];
uint assetIndex = len;
for (uint j = 0; j < len;) {
if (userHeldNFTs_[j] == nftID) {
assetIndex = j;
break;
}
unchecked { j++; }
}
if (assetIndex == len) {
revert NftNotFound(nftID);
}
underlying_.transferFrom(address(this), to, nftID);
// copy last item in list to location of item to be removed, reduce length by 1
len = len - 1;
userHeldNFTs_[assetIndex] = userHeldNFTs_[len];
userHeldNFTs_.pop();
unchecked { i++; }
}
Check for the index to be equal to the length and prevent redundant storing a value before deleting it. For example, change line 207 to:
if (assetIndex != len) {
userHeldNFTs_[assetIndex] = userHeldNFTs_[len];
}
FNG-21. CUSTOM ERROR
Severity:
Status:
Fixed
Path:
CErc721.sol:L318
Description:
In CERC721.sol on line 318 a validation check is performed using the revert function with a reason string.
if (redeemTokens == 0 && redeemAmount > 0) {
revert("redeemTokens zero");
}
Remediation:
Consider replacing this with a custom error to save on byte code size.