Lines of code

Vulnerability details

C4 issue

H-02: Risk of reentrancy onERC721Received function to manipulate collateral token configs shares

Comment

In the original code, the reentrancy happens in function V3Vault.onERC721Received, in case a new NFT is created during transformation, the code will call cleanupLoan which will send the old NFT back to the owner, this allows the attacker to re-enter the function by creating a onERC721Received hook:

function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
        external
        override
        returns (bytes4)
    {
    ...
     if (tokenId != oldTokenId) {
                address owner = tokenOwner[oldTokenId];

                // set transformed token to new one
                transformedTokenId = tokenId;

                // copy debt to new token
                loans[tokenId] = Loan(loans[oldTokenId].debtShares);

                _addTokenToOwner(owner, tokenId);
                emit Add(tokenId, owner, oldTokenId);

                // clears data of old loan
                _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);

                // sets data of new loan
                _updateAndCheckCollateral(
                    tokenId, debtExchangeRateX96, lendExchangeRateX96, 0, loans[tokenId].debtShares
                );
            }
    ...
    }
function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
        internal
    {
        _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
        emit Remove(tokenId, owner);
    }

Mitigation

PR #8, PR #32 The mitigation code remove the sending of old NFT to the owner:

function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) internal {
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
    }

This will prevent the reentrancy. The mitigation solved the original issue.

Conclusion

LGTM

code-423n4 / 2024-04-revert-mitigation-findings