First Flight #7: Horse Store
First Flight #7
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Reentrancy Vulnerability in `TRANSFER_TAKE_FROM()`

0xVinylDavyl

Summary

The contract reveals a potential reentrancy vulnerability in the TRANSFER_TAKE_FROM() function. If exploited, this vulnerability could lead to unintended behavior, allowing an attacker to manipulate the flow of execution by recursively calling the _MINT() function. The provided proof of concept includes a suggested modification to mitigate the issue.

Impact

An attacker, by repeatedly calling the _MINT() function within the TRANSFER_TAKE_FROM() context, can disrupt the expected execution flow. This may result in unexpected state changes, potentially leading to financial losses or undesired consequences within the smart contract.

Vulnerability Details

POC

// In TRANSFER_TAKE_FROM()
// ...
dup1 caller eq // [msg.sender == from, from, to, tokenId]
is_authorized jumpi // [from, to, tokenId]
// ... (existing code)
​
is_authorized:
// Update balance of from
0x01 dup2 // [from, 1, from, to, tokenId]
[BALANCE_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [balance, 1, from, to, tokenId]
sub dup2 // [from, balance-1, from, to, tokenId]
[BALANCE_LOCATION]
STORE_ELEMENT_FROM_KEYS(0x00) // [from, to, tokenId]
​
// ...
// Existing code
​
// Add reentrancy check
caller extcodesize is_reentrant jumpi // [from, to, tokenId]
stop
​
is_reentrant:
// Handle reentrancy (revert, log, or any suitable action)
REENTRANCY_ATTACK_DETECTED(0x00)

Tools Used

  • Manual code analysis

Recommendations

Vulnerability Details

// Original existing code snippet in TRANSFER_TAKE_FROM()
​
// If from !== ownerOf[tokenId] revert with "WRONG_FROM"
dup1 dup4 // [tokenId, from, from, to, tokenId]
[OWNER_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [owner, from, from, to, tokenId]
eq cont jumpi // [from, to, tokenId]
WRONG_FROM(0x00)
cont:
​
// If to === address(0) revert with "INVALID_RECIPIENT"
dup2 continue jumpi // [from, to, tokenId]
INVALID_RECIPIENT(0x00)
continue:
​
// Check if msg.sender == from
dup1 caller eq // [msg.sender == from, from, to, tokenId]
is_authorized jumpi // [from, to, tokenId]
​
// Check if msg.sender != from && !isApprovedForAll[from][msg.sender] && msg.sender != getApproved[id],
UNAUTHORIZED(0x00)
is_authorized:
​
// Update balance of from
0x01 dup2 // [from, 1, from, to, tokenId]
[BALANCE_LOCATION] LOAD_ELEMENT_FROM_KEYS(0x00) // [balance, 1, from, to, tokenId]
sub dup2 // [from, balance-1, from, to, tokenId]
[BALANCE_LOCATION]
STORE_ELEMENT_FROM_KEYS(0x00) // [from, to, tokenId]

Recommendations

The existing code checks for the correctness of the from and to addresses, ensuring they are valid and not equal to address(0). Additionally, it verifies that the msg.sender is authorized to perform the transfer. To mitigate the reentrancy vulnerability, the following recommendation is provided:

  1. Reentrancy Protection:
    Modify the existing code to include a reentrancy check, as demonstrated in the proof of concept. This involves checking whether the calling address (msg.sender) has any code (extcodesize). If it does, consider it potentially reentrant and take appropriate action (revert, log, etc.).

// Add reentrancy check
caller extcodesize is_reentrant jumpi // [from, to, tokenId]
stop
​
is_reentrant:
// Handle reentrancy (revert, log, or any suitable action)
REENTRANCY_ATTACK_DETECTED(0x00)

Modification adds a layer of protection against reentrancy attacks by stopping the execution if the caller has associated code.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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