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

Users can withdraw escrowed tokens to addresses they did not connect with the platform; breaking the logic

Baptizer

Summary

The usual logic of Ark is that users must first provide two addresses: an address to deposit tokens and a different address to withdraw tokens. However, tokens can be withdrawn to previously unconnected addresses.

Vulnerability Details

Here is the vulnerable function:

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
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;
}

There was no check to see if the address a user has added is the one they are transferring tokens to.

Impact

This vulnerability impacts tokens, as they can be bridged to a wallet outside of the two wallets the users provided. This breaks Ark's fundamental logic as an NFT bridge between Ethereum and Starknet.

Proof of Code

Add this function to the Escrow.t.solfile:

function setUp() public {
escrow = new EscrowPublic();
erc721 = address(new ERC721MintFree("name 1", "S1"));
​
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
​
vm.prank(alice);
IERC721(erc721).setApprovalForAll(address(escrow), true);
vm.prank(bob);
IERC721(erc721).setApprovalForAll(address(escrow), true);
}
​
function test_hackWithdraw() public {
IERC721MintRangeFree(erc721).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](2);
ids[0] = 5;
ids[1] = 8;
​
vm.startPrank(alice);
escrow.depositIntoEscrow(CollectionType.ERC721, erc721, ids);
bool wasInEscrow = escrow.withdrawFromEscrow(CollectionType.ERC721, erc721, address(0x2345), 5);
assertTrue(wasInEscrow);
vm.stopPrank();
​
assertEq(IERC721(erc721).ownerOf(5), address(0x2345));
}

In the code above, bobis the intended receiving address and is so registered in the contract. However, transfer to address(0x2345)was still successful.

Here is the result in Foundry:

Ran 1 test for test/Escrow.t.sol:EscrowTest
[PASS] test_hackWithdraw() (gas: 411525)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.34ms (538.90µs CPU time)

Tools Used

  • Manual review

  • Foundry

Recommendations

Add a check that only the receiving address that a user has connected can be the address to. This way, the logic of the contract can be maintained.

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.