Summary
The divideNft function in the TokenDivider contract has a critical input validation vulnerability where it attempts to perform operations on potentially invalid NFT addresses before validating them, leading to unsafe contract interactions.
Vulnerability Details
The vulnerability comes from the placement of validation checks and modifier usage in the divideNft function. The current implementation has two critical issues:
The function uses the onlyNftOwner modifier before validating the NFT address. This means the contract can attempt to interact with any address, including the zero address, before ensuring it's valid
modifier onlyNftOwner(address nft, uint256 tokenId) {
if(msg.sender != IERC721(nft).ownerOf(tokenId)) {
revert TokenDivider__NotFromNftOwner();
}
_;
}
function divideNft(address nftAddress, uint256 tokenId, uint256 amount)
onlyNftOwner(nftAddress, tokenId)
onlyNftOwner(nftAddress ,tokenId)
external {
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
...
}
The vulnerability was confirmed through invariant testing
pragma solidity ^0.8.18;
import {Test} from "forge-std/Test.sol";
import {StdInvariant} from "forge-std/StdInvariant.sol";
import {TokenDivider} from "../src/TokenDivider.sol";
import {ERC721Mock} from "./mocks/ERC721Mock.sol";
import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
contract TokenDividerInvariantTest is StdInvariant, Test {
TokenDivider public tokenDivider;
ERC721Mock public mockNft;
Handler public handler;
address public owner = makeAddr("owner");
address public user = makeAddr("user");
function setUp() public {
vm.startPrank(owner);
tokenDivider = new TokenDivider();
mockNft = new ERC721Mock();
handler = new Handler(tokenDivider, mockNft);
vm.stopPrank();
targetContract(address(handler));
}
function invariant_onlyOwnerCanDivideNFT() public {
assertEq(
handler.nftDivisionsByNonOwner(),
0,
"Non-owner successfully divided NFT"
);
}
function invariant_erc20SupplyMatchesDividedAmount() public {
assertEq(
handler.getTotalErc20Minted(),
handler.getTotalDividedAmount(),
"ERC20 supply doesn't match divided amount"
);
}
function invariant_nftTransferIsAtomic() public {
assertEq(
handler.getFailedTransfers(),
0,
"NFT transfer not atomic"
);
}
function invariant_noZeroAddressOperations() public {
assertEq(
handler.getZeroAddressAttempts(),
0,
"Zero address operation attempted"
);
}
}
contract Handler is Test {
TokenDivider public tokenDivider;
ERC721Mock public mockNft;
uint256 public nftDivisionsByNonOwner;
uint256 public totalErc20Minted;
uint256 public totalDividedAmount;
uint256 public failedTransfers;
uint256 public zeroAddressAttempts;
mapping(uint256 => address) public tokenIdToOwner;
uint256 public currentTokenId;
constructor(TokenDivider _tokenDivider, ERC721Mock _mockNft) {
tokenDivider = _tokenDivider;
mockNft = _mockNft;
}
function divideNft(
address nftHolder,
uint256 amount
) public {
if (nftHolder == address(0)) {
zeroAddressAttempts++;
return;
}
vm.startPrank(address(mockNft.owner()));
mockNft.mint(nftHolder);
uint256 tokenId = currentTokenId;
currentTokenId++;
tokenIdToOwner[tokenId] = nftHolder;
vm.stopPrank();
vm.startPrank(nftHolder);
mockNft.approve(address(tokenDivider), tokenId);
try tokenDivider.divideNft(
address(mockNft),
tokenId,
amount
) {
totalErc20Minted += amount;
totalDividedAmount += amount;
} catch {
failedTransfers++;
}
vm.stopPrank();
}
function divideNftAsNonOwner(
address nonOwner,
uint256 amount
) public {
if (nonOwner == address(0)) {
zeroAddressAttempts++;
return;
}
address actualOwner = makeAddr("actualOwner");
vm.startPrank(address(mockNft.owner()));
mockNft.mint(actualOwner);
uint256 tokenId = currentTokenId;
currentTokenId++;
vm.stopPrank();
vm.startPrank(nonOwner);
try tokenDivider.divideNft(
address(mockNft),
tokenId,
amount
) {
nftDivisionsByNonOwner++;
} catch {}
vm.stopPrank();
}
function getNftDivisionsByNonOwner() public view returns (uint256) {
return nftDivisionsByNonOwner;
}
function getTotalErc20Minted() public view returns (uint256) {
return totalErc20Minted;
}
function getTotalDividedAmount() public view returns (uint256) {
return totalDividedAmount;
}
function getFailedTransfers() public view returns (uint256) {
return failedTransfers;
}
function getZeroAddressAttempts() public view returns (uint256) {
return zeroAddressAttempts;
}
}
Result
[FAIL: Zero address operation attempted: 1 != 0]
[Sequence]
sender=0xd9dAcD352658c69BBC7CC342E44E8190d3ca670E
args=[0x0000000000000000000000000000000000000000, 157198260]
This shows that the contract attempts to perform operations on the zero address before any validation occurs, potentially leading to unexpected behaviors or reverts.
Impact
Invoking functions on the zero address or an invalid contract can lead to unpredictable behavior. An attacker could exploit this to cause reverts or potentially create scenarios that lock assets in the contract.
Unsafe interactions with invalid addresses can result in failed transactions. This could render the divideNft function unusable under certain conditions, leading to a disruption in protocol operations.
Interacting with invalid contracts without proper validation violates best practices for ERC721 and ERC20 standards, making the protocol incompatible with some external integrations.
Tools Used
Manual Review
Recommendations
Reorder the input validation to ensure that all parameters are checked before interacting with the NFT address.
function divideNft(address nftAddress, uint256 tokenId, uint256 amount) external {
if(nftAddress == address(0)) { revert TokenDivider__NftAddressIsZero(); }
if(amount == 0) { revert TokenDivider__AmountCantBeZero(); }
if(msg.sender != IERC721(nftAddress).ownerOf(tokenId)) {
revert TokenDivider__NotFromNftOwner();
}
if(!_isERC721(nftAddress)) {
revert TokenDivider__InvalidNFTContract();
}
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol())));
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) {
revert TokenDivider__NftTransferFailed();
}
balances[msg.sender][erc20] = amount;
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
erc20ToMintedAmount[erc20] = amount;
erc20ToNft[erc20] = nftAddress;
emit NftDivided(nftAddress, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if(!transferSuccess) {
revert TokenDivider__TransferFailed();
}
}
