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

Unsafe OneShot ERC721 NFT transfers in Streets.sol and misleading NatSpec comments. Reentrancy may occur in Streets.sol.

theirrationalone

Summary

The Streets::stake and Streets::unstake functions accept and transfer OneShot NFTs, posing a risk of reentrancy in Streets.sol. However, there are checks in place to defend against reentrancy attacks.

Vulnerability Details

Streets::stake function: (Not using safeTransferFrom function).

function stake(uint256 tokenId) external {
stakes[tokenId] = Stake(block.timestamp, msg.sender);
emit Staked(msg.sender, tokenId, block.timestamp);
// -------------------------
// ------------ ||
// ----- Vulnerable to reentrancy with no protection in place.
// ------ \/
@> oneShotContract.transferFrom(msg.sender, address(this), tokenId);
}

Streets:unstake function: (Not using safeTransferFrom function) and contains a misleading Natspec comment.

function unstake(uint256 tokenId) external {
...
...
...
​
// -------------------------
// ------------ ||
// --- // Misleading Natspec comment.
// ------ \/
@> // Assuming RapBattle contract has a function to update metadata properties
IOneShot.RapperStats memory stakedRapperStats = oneShotContract.getRapperStats(tokenId);
​
...
...
...
​
// ----------
// ----- ||
// -- // Have Protection against Reentrancy already.
// -- \/
@> require(stakes[tokenId].owner == msg.sender, "Not the token owner");
...
...
...
​
// Continue with unstaking logic (e.g., transferring the token back to the owner)
// -------------------------
// ------------ ||
// ----- not so safe transfer.
// ------ \/
@> oneShotContract.transferFrom(address(this), msg.sender, tokenId);
}

Streets.sol Misleading Natspec comment.

...
...
...
​
// --------
// --- ||
// There's also a ERC20 token contract `Credibility` adjacent to ERC721 token Interface `IOneShot`.
// - \/
@> // ERC721 token contract
IOneShot public oneShotContract;
Credibility public credContract;
​
...
...
...

Impact

  1. Unsafe Transfers: Both functions are susceptible to reentrancy attacks, although unstake has some protection against it (with minimal impact).

  2. Misleading NatSpecs:

    • The top first misleading NatSpec comment can confuse auditors, researchers, or other developers. It suggests that the ERC721 Token contract variables are declared there, but in reality, it's an ERC721 Token contract Interface. Adjacent to it, an ERC20 Token contract is declared. Auditors, researchers, or other developers, after seeing that NatSpec, may think both are ERC721 Token Contracts variables.

    • The provided NatSpec can mislead researchers, auditors, or other developers. It suggests the need for a RapBattle function to update OneShot NFT metadata. However, as Streets.sol already has privileges to update OneShot NFT metadata, invoking any RapBattle functions here is unnecessary.

Tools Used

Manual Review, Foundry.

Recommendations

  1. Utilize the safeTransferFrom function provided by OpenZeppelin::ERC721 instead of transferFrom.

  2. Remove or update the misleading NatSpec comments.

Updates

Lead Judging Commences

inallhonesty Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!