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

Violation of Checks-Effects-Interactions (CEI) programming pattern, leading to potential reentrancy attacks

Dowsers

Summary

Several functions in the Ark protocol do not adhere to the Checks-Effects-Interactions (CEI) programming pattern, making them vulnerable to reentrancy attacks. This could allow an attacker to manipulate the contract state in unintended ways, potentially disrupting the normal functioning of the bridge or causing inconsistencies between L1 and L2.

Vulnerability Details

The following functions do not respect the CEI pattern:

In Bridge.sol:

  • The depositTokens function performs external interactions before checking internal state:

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// ... (checks omitted for brevity)
​
@> _depositIntoEscrow(ctype, collectionL1, ids);
​
req.tokenIds = ids;
​
uint256[] memory payload = Protocol.requestSerialize(req);
if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

In Escrow.sol:

  • The _depositIntoEscrow function performs token transfers before updating the escrow state:

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
public
{
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);
} 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;
}
}

  • The _withdrawFromEscrow function performs token transfers before updating the escrow state:

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;
}

In Messaging.sol:

The _consumeMessageStarknet function interacts with the Starknet Core contract before checking internal state:

function _consumeMessageStarknet(
IStarknetMessaging starknetCore,
snaddress fromL2Address,
uint256[] memory request
)
internal
{
// Will revert if the message is not consumable.
@> bytes32 msgHash = starknetCore.consumeMessageFromL2(
snaddress.unwrap(fromL2Address),
request
);
​
// If the message were configured to be withdrawn with auto method,
// starknet method is denied.
if (_autoWithdrawn[msgHash] != WITHDRAW_AUTO_NONE) {
revert WithdrawMethodError();
}
}

Impact

The potential impact of this vulnerability is significant. While the bridge only handles NFTs, an attacker could still exploit this flaw to manipulate the bridge state in unintended ways, disrupting the normal functioning of the protocol:

An attacker could create a malicious NFT contract that, when transferred, calls back into the depositTokens function. This could allow the attacker to deposit the same NFT multiple times before the escrow state is updated, potentially creating multiple bridge requests for a single NFT. This could lead to inconsistencies between L1 and L2, where the L2 side might mint multiple copies of the same NFT.

An attacker could execute L2 messages multiple times, leading to state inconsistencies between L1 and L2:

If an attacker can control an NFT contract on L1, they could create a withdrawTokens function that, when called, triggers a reentrancy attack on the _consumeMessageStarknet function. This could allow the attacker to consume the same L2 message multiple times before the _autoWithdrawn state is updated. As a result, the attacker might be able to mint multiple NFTs on L1 for a single withdrawal request from L2, effectively duplicating NFTs during the bridging process.

These scenarios could lead to severe disruptions in the bridge's operation, potentially allowing unauthorized minting or duplication of NFTs, and causing a loss of trust in the protocol.

Tools Used

Manual review - Foundry IDE

Recommendations

To resolve this vulnerability, it is recommended to restructure the mentioned functions to follow the CEI pattern:

  1. Perform all necessary checks first.

  2. Update the internal state of the contract.

  3. Perform external interactions last.

For example, the depositTokens function in Bridge.sol could be restructured as follows:

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// ... (checks omitted for brevity)
​
--- _depositIntoEscrow(ctype, collectionL1, ids);
​
req.tokenIds = ids;
​
uint256[] memory payload = Protocol.requestSerialize(req);
if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
+++ _depositIntoEscrow(ctype, collectionL1, ids);
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

For example, the _depositIntoEscrow function in Escrow.sol could be restructured as follows:

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
public
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
+++ _escrow[collection][id] = msg.sender;
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(msg.sender, address(this), id);
} 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;
}
}

For example, the _withdrawFromEscrow function in Escrow.sol could be restructured as follows:

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
// Checks
if (!_isEscrowed(collection, id)) {
return false;
}
​
// Effects
+++ _escrow[collection][id] = address(0x0);
​
// Interactions
address from = address(this);
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
--- _escrow[collection][id] = address(0x0);
​
return true;
}

For example, the `_consumeMessageStarknet` function in Messaging.sol could be restructured as follows:

function _consumeMessageStarknet(
IStarknetMessaging starknetCore,
snaddress fromL2Address,
uint256[] memory request
)
internal
{
// Will revert if the message is not consumable.
--- bytes32 msgHash = starknetCore.consumeMessageFromL2(
--- snaddress.unwrap(fromL2Address),
--- request
--- );
​
// If the message were configured to be withdrawn with auto method,
// starknet method is denied.
if (_autoWithdrawn[msgHash] != WITHDRAW_AUTO_NONE) {
revert WithdrawMethodError();
}
+++ bytes32 msgHash = starknetCore.consumeMessageFromL2(
+++ snaddress.unwrap(fromL2Address),
+++ request
+++ );
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
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.