First Flight #10: One Shot
First Flight #10
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Unsafe use of `ERC721.transferFrom()` instead of `safeTransferFrom()` risks freezing of OneShot Rapper NFTs with incompatible `ERC-721` contracts.

oxenzo

Description: In the RapBattle contract,it doesn't implement the onERC721Received function, thus making the OneShot NFT to be lost in the blockchain. The RapBattle::goOnStageOrBattle function is making NFT transfers to the RapBattle contract as shown below;

function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
if (defender == address(0)) {
defender = msg.sender;
defenderBet = _credBet;
defenderTokenId = _tokenId
emit OnStage(msg.sender, _tokenId, _credBet);
// The defender's nft is sent to this RapBattle contract which
// doesnt have a `onERC721Received` function
@> oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
​
credToken.transferFrom(msg.sender, address(this), _credBet);
} else {
_battle(_tokenId, _credBet);
}
}
​

Moreover, In the Streets::unstake function, the _to refers to msg.sender. However, if _to is a contract address that does not support ERC721, the NFT can be frozen in the contract same as in the RapBattle::_battle function.

The Streets::unstake function;

​
​
function unstake(uint256 tokenId) external {
require(stakes[tokenId].owner == msg.sender, "Not the token owner");
uint256 stakedDuration = block.timestamp - stakes[tokenId].startTime;
uint256 daysStaked = stakedDuration / 1 days;
​
IOneShot.RapperStats memory stakedRapperStats = oneShotContract.getRapperStats(tokenId);
​
emit Unstaked(msg.sender, tokenId, stakedDuration);
​
delete stakes[tokenId];
​
if (daysStaked >= 1) {
stakedRapperStats.weakKnees = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 2) {
stakedRapperStats.heavyArms = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 3) {
stakedRapperStats.spaghettiSweater = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 4) {
stakedRapperStats.calmAndReady = true;
credContract.mint(msg.sender, 1);
}
​
if (daysStaked >= 1) {
oneShotContract.updateRapperStats(
tokenId,
stakedRapperStats.weakKnees,
stakedRapperStats.heavyArms,
stakedRapperStats.spaghettiSweater,
stakedRapperStats.calmAndReady,
stakedRapperStats.battlesWon
);
}
​
@> oneShotContract.transferFrom(address(this), msg.sender, tokenId);
}
​

The RapBattle::_battle function;

​
function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
uint256 challengerRapperSkill = getRapperSkill(_tokenId);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
uint256 totalPrize = defenderBet + _credBet;
​
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;
​
defender = address(0);
emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
​
if (random <= defenderRapperSkill) {
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
// Return the defender's NFT
@> oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
}
​

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Alternatively, you could read up about this here; EIP-721 documentation.

Impact: The NFT may get stuck in the contract that does not support ERC-721 tokens.

Recommended Mitigation: Make sure in the RapBattle contract, it implements the onERC721Received function so it can be able to handle NFTs being sent to it as shown below;

contract RapBattle {
// Rest of Code ........
​
+ function onERC721Received(address, address, uint256, bytes calldata) external pure override returns (bytes4) {
+ return IERC721Receiver.onERC721Received.selector;
+ }
+ }
​

Moreover, consider using SafeTransferFrom() instead of transferFrom() in all the corresponding functions so as to accept safe transfers.

function unstake(uint256 tokenId) external {
require(stakes[tokenId].owner == msg.sender, "Not the token owner");
uint256 stakedDuration = block.timestamp - stakes[tokenId].startTime;
uint256 daysStaked = stakedDuration / 1 days;
​
IOneShot.RapperStats memory stakedRapperStats = oneShotContract.getRapperStats(tokenId);
​
emit Unstaked(msg.sender, tokenId, stakedDuration);
​
delete stakes[tokenId];
​
if (daysStaked >= 1) {
stakedRapperStats.weakKnees = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 2) {
stakedRapperStats.heavyArms = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 3) {
stakedRapperStats.spaghettiSweater = false;
credContract.mint(msg.sender, 1);
}
if (daysStaked >= 4) {
stakedRapperStats.calmAndReady = true;
credContract.mint(msg.sender, 1);
}
​
if (daysStaked >= 1) {
oneShotContract.updateRapperStats(
tokenId,
stakedRapperStats.weakKnees,
stakedRapperStats.heavyArms,
stakedRapperStats.spaghettiSweater,
stakedRapperStats.calmAndReady,
stakedRapperStats.battlesWon
);
}
​
- oneShotContract.transferFrom(address(this), msg.sender, tokenId);
+ oneShotContract.safeTransferFrom(address(this), msg.sender, tokenId);
}
​
​
function _battle(uint256 _tokenId, uint256 _credBet) internal {
address _defender = defender;
require(defenderBet == _credBet, "RapBattle: Bet amounts do not match");
uint256 defenderRapperSkill = getRapperSkill(defenderTokenId);
uint256 challengerRapperSkill = getRapperSkill(_tokenId);
uint256 totalBattleSkill = defenderRapperSkill + challengerRapperSkill;
uint256 totalPrize = defenderBet + _credBet;
​
uint256 random =
uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender))) % totalBattleSkill;
​
defender = address(0);
emit Battle(msg.sender, _tokenId, random < defenderRapperSkill ? _defender : msg.sender);
​
if (random <= defenderRapperSkill) {
credToken.transfer(_defender, defenderBet);
credToken.transferFrom(msg.sender, _defender, _credBet);
} else {
credToken.transfer(msg.sender, _credBet);
}
totalPrize = 0;
// Return the defender's NFT
- oneShotNft.transferFrom(address(this), _defender, defenderTokenId);
+ oneShotNft.safeTransferFrom(address(this), _defender, defenderTokenId);
}
​
function goOnStageOrBattle(uint256 _tokenId, uint256 _credBet) external {
if (defender == address(0)) {
defender = msg.sender;
defenderBet = _credBet;
defenderTokenId = _tokenId
emit OnStage(msg.sender, _tokenId, _credBet);
// The defender's nft is sent to this RapBattle contract which
// doesnt have a `onERC721Received` function
- oneShotNft.transferFrom(msg.sender, address(this), _tokenId);
+ oneShotNft.safeTransferFrom(msg.sender, address(this), _tokenId);
​
credToken.transferFrom(msg.sender, address(this), _credBet);
} else {
_battle(_tokenId, _credBet);
}
}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
oxenzo Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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