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

Lack of check for the minimum fee on L1 when bridging to L2 can often cause the L2 execution to revert.

said

Summary

When users call depositTokens to bridge their NFTs to L2, they can provide any amount of msg.value as the fee for the execution. This could become problematic, as the bridging process may frequently fail due to insufficient fees.

Vulnerability Details

It can be observed that depositTokens will accept any amount of fee (msg.value) and send the L1 -> L2 message to the StarkNet core address.

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

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// ...
>>> IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

StarkNet core address will accept any non 0 msg.value as long as it not exceed max fee.

https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/starknet/solidity/StarknetMessaging.sol#L110-L125

function sendMessageToL2(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload
) external payable override returns (bytes32, uint256) {
require(msg.value > 0, "L1_MSG_FEE_MUST_BE_GREATER_THAN_0");
require(msg.value <= getMaxL1MsgFee(), "MAX_L1_MSG_FEE_EXCEEDED");
uint256 nonce = l1ToL2MessageNonce();
NamedStorage.setUintValue(L1L2_MESSAGE_NONCE_TAG, nonce + 1);
emit LogMessageToL2(msg.sender, toAddress, selector, payload, nonce, msg.value);
bytes32 msgHash = getL1ToL2MsgHash(toAddress, selector, payload, nonce);
// Note that the inclusion of the unique nonce in the message hash implies that
// l1ToL2Messages()[msgHash] was not accessed before.
l1ToL2Messages()[msgHash] = msg.value + 1;
return (msgHash, nonce);
}

To understand the potential issue, let's first examine how L1 -> L2 messaging is executed and how the fee is deducted.

After the transaction from L1 is send to the L2, then sequencer executed the target function with l1_handler on the L2 target contract. The sequencer will add the execution to the proof, then will include it in Core contract state update on L1. Then the message is cleared from the Core Contract’s storage to consume the message. The sequencer charges the fee in full upon updating the L1 state with the consumption of this message.

The fee that will be charged is calculated using the same method as regular L2 transactions. This means that if there is more computation or operation on the L2 side (such as providing multiple token IDs), a larger fee will need to be provided when calling sendMessageToL2.

reference : here

it is part of the StarkNet roadmap to implement and enforce proper fees for executing L1 to L2 transactions.

Impact

Not providing correct fee could cause execution on L2 to fail.

Tools Used

Manual review

Recommendations

Provide a proper minimum fee check when depositTokens is called.

Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-not-enough-fee-can-block-NFT

Impact: Medium/High. Need an admin to start a cancellation and wait for 5 days once done. DoS > 5 days. Likelyhood: Low. Everytime a wallet/or a user do not send enough gas

Support

FAQs

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