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

Unprecise message fee mechanism can lead to rejected messages or locked funds

4eyes

Summary

The depositTokens function in Bridge.sol, which takes care for L1 to L2 token deposits, lacks a proper fee mechanism to handle the message sending between Layer 1 (Ethereum) and Layer 2 (Starknet). According to Starknet's documentation, the sendMessageToL2 function should ensure accurate fee estimation and collection for L1 → L2 messages.

https://docs.starknet.io/architecture-and-concepts/network-architecture/messaging-mechanism/#:~:text=the cancelL1ToL2Message function.-,L1 → L2 message fees,CLI to get an estimate of an L1 → L2 message fee.,-L1 → L2 structure

However, the current implementation does not follow the recommended fee-handling mechanism.

Code Snippet

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

Vulnerability Details

The depositTokens function in Bridge.sol sends a message to L2 using the sendMessageToL2 function, which collects a fee for processing the L1 → L2 transaction. However, the implementation does not adequately handle the fee mechanism as recommended by Starknet:

  1. The function does not dynamically calculate or estimate the required fee for L1 → L2 transactions, which is critical in ensuring the message is processed by the sequencer. Instead, it only checks whether the fee (msg.value) is greater than zero and does not exceed the maximum allowed (getMaxL1MsgFee()).

  2. Without dynamically calculating the fee, there is no guarantee that the user pays enough to incentivize the sequencer to include the transaction. This could result in the L1 → L2 message being rejected due to underpayment, leaving the transaction unprocessed.

  3. Conversely, if the user overestimates the required fee, there is no refund mechanism in place, leading to excess ETH being locked.

Impact

The absence of a proper fee mechanism can lead to underpayment or overpayment respectively to message being rejected or locked funds.

Tools Used

Manual review

Recommendations

Consider implementing a fee mechanism for L1->L2 messages.

Updates

Lead Judging Commences

n0kto Lead Judge 10 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.