ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Deposits are executed with `transferFrom` instead of `safeTransferFRom`.

lightoasis

Summary

Deposits are executed with transferFrom instead of safeTransferFRom.

Vulnerability Details

Deposits are executed with transferFrom instead of safeTransferFRom. The difference between transferFrom and safeTransferFrom in ERC721 is the callback that safeTransferFrom performs to ensure that the receiver is a safe recipient. While transferFrom and safeTransferFrom are both used to transfer a token from one account to another, safeTransferFrom checks if the recipient is a contract and if so, it calls a callback function on the recipient, onERC721Received, and reverts if the recipient does not return the magic value. The protocol should consider using safeTransferFRom to transfer the ERC721 tokens.

Proof OF Concept

As seen below, transferFrom is used instead of safeTransferFRom to execute deposits.
https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Escrow.sol#L39

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
if (collectionType == CollectionType.ERC721) {
@> IERC721(collection).transferFrom(msg.sender, address(this), id);
}
}
}

References:

https://coinsbench.com/rareskills-solidity-interview-question-26-answered-what-is-the-difference-between-transferfrom-8a1fb636fcd1

https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#IERC721-safeTransferFrom-address-address-uint256-bytes-

Impact

Deposits are executed with transferFrom instead of safeTransferFRom.

Tools Used

Manual Review

Recommendations

The protocol should use safeTransferFrom to execute deposits.

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
if (collectionType == CollectionType.ERC721) {
+ IERC721(collection).safeTransferFrom(msg.sender, address(this), id);
}
}
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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