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

frontrunning in depositTokens function

kwakudr

Summary

The use of a user-provided salt for generating the request hash could be exploited by malicious actors to front-run transactions

Vulnerability Details

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L78C5-L145C1

req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids);
function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
if (!Cairo.isFelt252(snaddress.unwrap(ownerL2))) {
revert CairoWrapError();
}
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
​
CollectionType ctype = TokenUtil.detectInterface(collectionL1);
if (ctype == CollectionType.ERC1155) {
revert NotSupportedYetError();
}
​
​
if (!_isWhiteListed(collectionL1)) {
revert NotWhiteListedError();
}
​
​
Request memory req;
​
​
// The withdraw auto is only available for request originated from
// Starknet side as the withdraw on starknet is automatically done
// by the sequencer.
req.header = Protocol.requestHeaderV1(ctype, useAutoBurn, false);
req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids);
// TODO: store request hash in storage to avoid replay attack.
// or can it be safe to use block timestamp? Not sure as
// several tx may have the exact same block.
req.collectionL1 = collectionL1;
req.collectionL2 = _l1ToL2Addresses[collectionL1];
​
​
req.ownerL1 = msg.sender;
req.ownerL2 = ownerL2;
​
​
if (ctype == CollectionType.ERC721) {
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil.erc721Metadata(
collectionL1,
ids
);
} else {
(req.uri) = TokenUtil.erc1155Metadata(collectionL1);
}
​
​
_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);
}

Let's say we have two users: Alice (honest user) and Bob (malicious actor).

  1. Alice wants to deposit a valuable NFT (ID: 42) from collection 0x123... to L2 address 0x456...

  2. Alice prepares a transaction with these parameters:

    salt: 1234
    collectionL1: 0x123...
    ownerL2: 0x456...
    ids: [42]. Alice submits this transaction to the Ethereum mempool.

  4. Bob, running a bot that monitors the mempool, sees Alice's transaction.

  5. Bob quickly prepares a similar transaction with the same parameters, but changes the ownerL2 to his own address:salt: 1234
    collectionL1: 0x123...
    ownerL2: 0x789... (Bob's address)
    ids: [42]

  6. Bob submits his transaction with a higher gas price, ensuring it's mined before Alice's transaction.

  7. Bob's transaction is processed first:

    • The request hash is generated using Bob's parameters.

    • The NFT is transferred to the bridge contract.

    • A message is sent to L2 to mint the NFT to Bob's L2 address.

  8. When Alice's transaction is processed:

    • It will revert due to the NFT no longer being in Alice's possession.

In this scenario, Bob has successfully front-run Alice's transaction and stolen her NFT. This is because The salt is user-provided and visible in the mempool. The request hash doesn't include any user-specific data that can't be replicated by an attacker. There's no mechanism to ensure that the person who initiated the deposit is the same person who owned the NFT.

Impact

frontrun attack possible. Poor Alice NFT will be stolen

Tools Used

Manual Review

Recommendations

Use commit reveal scheme. OR

req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids, msg.sender);OR

req.hash = Protocol.requestHash(userNonce[msg.sender]++, collectionL1, ownerL2, ids);

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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