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

The `Escrow::_withdrawFromEscrow` allows a user to withdraw tokens from escrow even if the token does not belong to them

spomaria

Summary

The Escrow::_withdrawFromEscrow function is designed to allow a user withdraw tokens they deposited into the escrow. However, this function only checks if the token to be withdrawn was deposited in the escrow without checking if the user attempting to withdraw the token is the owner of the token or not. Hence, just anyone can withdraw token from escrow.

Vulnerability Details

The vulnerability of the Escrow::_withdrawFromEscrow function lies here

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
@> if (!_isEscrowed(collection, id)) {
return false;
}

The above code snippet can further be verified at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Escrow.sol#L63-L89

Notice that the _withdrawFromEscrow calls _isEscrowed which only checks if the token was deposited in the escrow as can be seen in the code snippet below

function _isEscrowed(
address collection,
uint256 id
)
internal
view
returns (bool)
{
return _escrow[collection][id] > address(0x0);
}

The above code snippet can further be verified at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Escrow.sol#L99-L108

Thus, just anyone can withdraw a token from escrow in so far as the token is in the escrow.

Impact

Because the Escrow::_withdrawFromEscrow function does not check if the caller is the owner of the token in escrow or not, any user who is aware that a token has been deposited in escrow can withdraw that token leading to loss of user tokens and funds.

Tools Used

Manual Review

Foundry

Proof of Concept:

  1. User A deposits their token in escrow

  2. User B notices that a token has been deposited in escrow and withdraws the token from escrow

  3. Now the token belongs to user B and user A losses their token and funds

PoC Place the following code into `Escrow.t.sol`.
function test_CanWithdrawFromEscrowEvenIfNotOwnerOfNFT() public {
IERC721MintRangeFree(erc721).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](2);
ids[0] = 5;
ids[1] = 8;
​
// allice deposits her NFT in escrow
vm.prank(alice);
escrow.depositIntoEscrow(CollectionType.ERC721, erc721, ids);
​
// bob notices that alice has deposited her NFT in escrow and withdraws it
vm.startPrank(bob);
bool wasInEscrow = escrow.withdrawFromEscrow(CollectionType.ERC721, erc721, bob, 5);
assertTrue(wasInEscrow);
vm.stopPrank();
​
assertEq(IERC721(erc721).ownerOf(5), bob);
}

Recommendations

  1. Consider modifying the -withdrawFromEscrow function to check if the caller is the owner of the token.

+ error Escrow__NotOwner();
​
.
.
.
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
+ if(_escrow[collection][id] != msg.sender) {
+ revert Escrow__NotOwner();
+ }
​
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
_escrow[collection][id] = address(0x0);
​
return true;
}

This will ensure that only the owner can withdraw a token from escrow thus, making the protocol safer for users and keeping user tokens safe.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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