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

Missing check when withdrawing from escrow during cancellation

fyamf

Summary

The returned Boolean from the function _withdrawFromEscrow is not checked, leading to a situation that the protocol considers a cancellation procedure as valid even if for any reason the token is not escrowed on L1. So, the protocol wrongly assumes the cancellation was successful, but in reality no NFT is transferred/withdrawn to the owner.

Vulnerability Details

When cancelling a request, if for any reason the NFT is not escrowed on L1 (due to other bugs), then withdrawal from escrow should be reverted. But since the returned Boolean from the function _withdrawFromEscrow is not checked, it will consider the cancellation as successful without any real NFT transfer to the owner.

function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
_withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
}
}

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

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

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

Moreover, since starknet consumes its cancellation, it will not be retriable anymore.

function cancelL1ToL2Message(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload,
uint256 nonce
) external override returns (bytes32) {
emit MessageToL2Canceled(msg.sender, toAddress, selector, payload, nonce);
// Note that the message hash depends on msg.sender, which prevents one contract from
// cancelling another contract's message.
// Trying to do so will result in NO_MESSAGE_TO_CANCEL.
bytes32 msgHash = getL1ToL2MsgHash(toAddress, selector, payload, nonce);
uint256 msgFeePlusOne = l1ToL2Messages()[msgHash];
require(msgFeePlusOne != 0, "NO_MESSAGE_TO_CANCEL");
​
uint256 requestTime = l1ToL2MessageCancellations()[msgHash];
require(requestTime != 0, "MESSAGE_CANCELLATION_NOT_REQUESTED");
​
uint256 cancelAllowedTime = requestTime + messageCancellationDelay();
require(cancelAllowedTime >= requestTime, "CANCEL_ALLOWED_TIME_OVERFLOW");
require(block.timestamp >= cancelAllowedTime, "MESSAGE_CANCELLATION_NOT_ALLOWED_YET");
​
l1ToL2Messages()[msgHash] = 0;
return (msgHash);
}

https://github.com/starkware-libs/cairo-lang/blob/v0.13.2a0/src/starkware/starknet/solidity/StarknetMessaging.sol#L182

Impact

  • Wrongly considering a cancellation as successful even though no NFT is withdrawn to the owner.

Tools Used

Recommendations

function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
- _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
+ require(_withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id), "not successful");
}
}
Updates

Lead Judging Commences

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

invalid-_withdrawFromEscrow-result-not-checked

To cancel a message, it has to be sent to the Starknet Core, otherwise it reverts. Therefore, to cancel a request, a token will always be escrowed. There is no impact here because the described case will never happen, that’s why check that boolean is not useful.

Support

FAQs

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