First Flight #17: Dussehra
First Flight #17
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

Unupdated internal accounting when NFTs are transferred via `ERC721` functions

paprikrumplikas

Summary

The ram field of the RamNFT::CharacteristicsOfRam struct is not updated when an NFT is transferred from one user to another using ERC721 functions.

Vulnerability Details

The ram field of the RamNFT::CharacteristicsOfRam struct is supposed to hold the address of the owner of the NFT. This field is first populated when an NFT is minted via Dussehra::enterPeopleWhoLikeRam, and is then used at several other places:

  • in ChoosingRam::increaseValuesOfParticipants, to specify the selected Ram,

  • in ChoosingRam::selectRamIfNotSelected, to specify the selected Ram,

  • indirectly in Dussehra::withdraw, which is accessible only for the selected Ram.

However, the protocol does not take into account that users can sell/buy or transfer Ram NFTs between each other, and does not update the CharacteristicsOfRam.ram field when ownership changes.

This is demonstarted by the following test:

function test_NftTransferred() public {
vm.deal(player1, 1 ether);
​
vm.startPrank(player1);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
ramNFT.safeTransferFrom(player1, player2, 0);
vm.stopPrank();
​
// player2 is the owner
assertEq(ramNFT.ownerOf(0), player2);
// but getCharacteristics.ram still holds player1
assertEq(ramNFT.getCharacteristics(0).ram, player1);
}

Impact

If userA calls Dussehra::enterPeopleWhoLikeRam and gets a Ram NFT with tokenId = x, but then sells/transfers this NFT to userB, then

  • userA can still call ChooseRam::increaseValuesOfParticipants as if the NFT with tokenId = x was still his,

  • if tokenId = x gets selected as Ram, userA will get 50% of the prize pool, not userB.

Tools Used

Manual review, Foundry.

Recommendations

Ensure that CharacteristicsOfRam.ram is modified on transfers by modifying RamNFT as follows:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
​
import {ERC721URIStorage, ERC721} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
+ import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
​
contract RamNFT is ERC721URIStorage {
error RamNFT__NotOrganiser();
error RamNFT__NotChoosingRamContract();
​
// https://medium.com/illumination/16-divine-qualities-of-lord-rama-24c326bd6048
struct CharacteristicsOfRam {
address ram;
bool isJitaKrodhah;
bool isDhyutimaan;
bool isVidvaan;
bool isAatmavan;
bool isSatyavaakyah;
}
​
uint256 public tokenCounter;
address public organiser;
address public choosingRamContract;
​
mapping(uint256 tokenId => CharacteristicsOfRam) public Characteristics;
​
modifier onlyOrganiser() {
if (msg.sender != organiser) {
revert RamNFT__NotOrganiser();
}
_;
}
​
modifier onlyChoosingRamContract() {
if (msg.sender != choosingRamContract) {
revert RamNFT__NotChoosingRamContract();
}
_;
}
​
constructor() ERC721("RamNFT", "RAM") {
tokenCounter = 0;
organiser = msg.sender;
}
​
+ function transferFrom(address from, address to, uint256 tokenId) public override(IERC721, ERC721) {
+ Characteristics[tokenId].ram = to;
+ super.transferFrom(from, to, tokenId);
+ }
​
function setChoosingRamContract(address _choosingRamContract) public onlyOrganiser {
choosingRamContract = _choosingRamContract;
}
function mintRamNFT(address to) public {
uint256 newTokenId = tokenCounter++;
_safeMint(to, newTokenId);
​
Characteristics[newTokenId] = CharacteristicsOfRam({
ram: to,
isJitaKrodhah: false,
isDhyutimaan: false,
isVidvaan: false,
isAatmavan: false,
isSatyavaakyah: false
});
}
​
function updateCharacteristics(
uint256 tokenId,
bool _isJitaKrodhah,
bool _isDhyutimaan,
bool _isVidvaan,
bool _isAatmavan,
bool _isSatyavaakyah
) public onlyChoosingRamContract {
​
Characteristics[tokenId] = CharacteristicsOfRam({
ram: Characteristics[tokenId].ram,
isJitaKrodhah: _isJitaKrodhah,
isDhyutimaan: _isDhyutimaan,
isVidvaan: _isVidvaan,
isAatmavan: _isAatmavan,
isSatyavaakyah: _isSatyavaakyah
});
}
​
function getCharacteristics(uint256 tokenId) public view returns (CharacteristicsOfRam memory) {
return Characteristics[tokenId];
}
function getNextTokenId() public view returns (uint256) {
return tokenCounter;
}
}
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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