Summary
Shop partners can mint cats to arbitrary users.
Vulnerability Details
Docs say users need to buy cats in order to get an NFT minted, but there is no state variable which shows users who should receive an NFT. Furthermore, the KittyConnect::mintCatToNewOwner function allows a new catOwner who might not have bought a cat, therefore they could receive an NFT without paying.
Impact
Users could receive undefined number of NFTs without paying if desired by shop partners, these could inflate the number of active NFTs and thus decrease their value.
Tools Used
Manual review
Foundry testing
POC:
pragma solidity 0.8.19;
import {Test, console} from "forge-std/Test.sol";
import {DeployKittyConnect} from "../script/DeployKittyConnect.s.sol";
import {HelperConfig} from "../script/HelperConfig.s.sol";
import {KittyConnect} from "../src/KittyConnect.sol";
import {KittyBridge, KittyBridgeBase, Client} from "../src/KittyBridge.sol";
contract KittyTest is Test {
KittyConnect kittyConnect;
KittyBridge kittyBridge;
HelperConfig helperConfig;
HelperConfig.NetworkConfig networkConfig;
address kittyConnectOwner;
address partnerA;
address partnerB;
address user;
address ethUsdPriceFeed;
event ShopPartnerAdded(address partner);
event CatMinted(uint256 tokenId, string catIpfsHash);
event TokensRedeemedForVetVisit(
uint256 tokenId,
uint256 amount,
string remarks
);
event CatTransferredToNewOwner(
address prevOwner,
address newOwner,
uint256 tokenId
);
function setUp() external {
DeployKittyConnect deployer = new DeployKittyConnect();
(kittyConnect, helperConfig) = deployer.run();
networkConfig = helperConfig.getNetworkConfig();
kittyConnectOwner = kittyConnect.getKittyConnectOwner();
partnerA = kittyConnect.getKittyShopAtIdx(0);
partnerB = kittyConnect.getKittyShopAtIdx(1);
kittyBridge = KittyBridge(kittyConnect.getKittyBridge());
user = makeAddr("user");
}
function test_mintUnlimitedCats() public{
vm.startPrank(partnerA);
kittyConnect.mintCatToNewOwner(user, "hash", "Toby", "littleCat", 7);
kittyConnect.mintCatToNewOwner(user, "hash", "Toby", "littleCat", 18);
kittyConnect.mintCatToNewOwner(user, "hash", "Toby", "littleCat", 35);
kittyConnect.mintCatToNewOwner(user, "hash", "Toby", "littleCat", 7);
assertEq(kittyConnect.getTokenCounter(), 4);
}
}
Recommendations
Using a mapping to tell whether a user can receive an NFT or not, there should be a function for a user to buy the NFT as well:
contract KittyConnect is ERC721 {
.
.
.
mapping(address => bool) private s_isKittyShop;
address[] private s_kittyShops;
mapping(address user => uint256[]) private s_ownerToCatsTokenId;
+ mapping(address => bool) private s_canReceiveCat;
mapping(uint256 tokenId => CatInfo) private s_catInfo;
.
.
.
+function buyCat() external payable{
+if(msg.value > 1 ether) s_canReceiveCat[msg.sender] = true;
+}
.
.
.
function mintCatToNewOwner(address catOwner, string memory catIpfsHash, string memory catName, string memory breed, uint256 dob) external onlyShopPartner {
require(!s_isKittyShop[catOwner], "KittyConnect__CatOwnerCantBeShopPartner");
+ require(s_canReceiveCat[catOwner], "This user has not bought any cats!");
+ s_canReceiveCat[catOwner] = false;
uint256 tokenId = kittyTokenCounter;
kittyTokenCounter++;
s_catInfo[tokenId] = CatInfo({
catName: catName,
breed: breed,
image: catIpfsHash,
dob: dob,
prevOwner: new address[](0),
shopPartner: msg.sender,
idx: s_ownerToCatsTokenId[catOwner].length
});
s_ownerToCatsTokenId[catOwner].push(tokenId);
_safeMint(catOwner, tokenId);
emit CatMinted(tokenId, catIpfsHash);
}
