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

Re-entrancy on withdrawTokens() function

CryptoAudit

Summary

OpenZeppelin's ERC721.sol includes callback functions to manage NFTs and prevent them from getting stuck in contracts. For NFT contracts, there exist some implicit external function calls that could be neglected by developers. They include onERC721Received function. The onERC721Received function was designed to check whether the receiver contract can handle NFTs. This function is invoked in the safeTransferFrom() of the ERC721 contract. Due to this external function call, the reentrancy could happen without being noticed by contract developer.

Vulnerability Details

According to what I said above, the malicious person can abuse the same things and write in the onERC721Received function of his contract to call the withdrawTokens() function many times in a loop.

In the withdrawTokens() function of the Bridge.sol contract, the _withdrawFromEscrow() function is called.

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L201

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
...
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);

safeTransferFrom is called in the _withdrawFromEscrow function

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L79

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
...
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
}

POC :

  1. Attacker calls the withdrawTokens() function.

  2. Due to the lack of Reentrancy Guard adherence, the attacker re-enters the withdrawTokens() function in onERC721Received before the initial call completes.

  3. This process can be repeated, allowing the attacker to withdraw token/NFT more than their allowed.

The same explanation above is also true for the following.

Actually, the onERC1155Received callback function is invoked in ERC1155.safeTransferFrom.

In addition, there is also the problem of lack of CEI pattern here

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L129

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
...
​
_depositIntoEscrow(ctype, collectionL1, ids);
​
req.tokenIds = ids;
​
uint256[] memory payload = Protocol.requestSerialize(req);
if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L46

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];
...
} else {
// TODO: check the supply is exactly one.
// (this is the low level call to verify if a contract has some function).
// (but it's better to check with supported interfaces? It's 2 calls instead
// of one where we control the fail.)
//(bool success, bytes memory data) = contractAddress.call("");
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
}
​
_escrow[collection][id] = msg.sender;
}

References

https://samczsun.com/the-dangers-of-surprising-code/

https://blog.openzeppelin.com/reentrancy-after-istanbul

https://eips.ethereum.org/EIPS/eip-721

https://docs.openzeppelin.com/contracts/3.x/api/token/erc1155

https://medium.com/amber-group/position-exchanges-re-entrancy-loophole-explained-ef176a0fd987

https://medium.com/immunefi/hack-analysis-omni-protocol-july-2022-2d35091a0109

https://solodit.xyz/issues/re-entrancy-issue-for-erc1155-fixed-consensys-bridge-mutual-markdown

https://solodit.xyz/issues/h-03-an-attacker-can-hijack-any-erc1155-token-he-rents-due-to-a-design-issue-in-renft-via-reentrancy-exploitation-code4rena-renft-renft-git

https://www.rareskills.io/post/where-to-find-solidity-reentrancy-attacks

https://blocksecteam.medium.com/revest-finance-vulnerabilities-more-than-re-entrancy-1609957b742f

Impact

An attacker can withdraw a large amount of tokens/NFTs using reentrancy.

Tools Used

Recommendations

Add a reentrancy guard (nonReentrant())

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Lack of quality
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Appeal created

CryptoAudit Submitter
12 months ago
n0kto Lead Judge
12 months ago
n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Support

FAQs

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