Summary
Bridge::depositTokens() lacks the evaluation and check of msg.value, which may result in the loss of ETH and the NFT being locked in the contract. Additionally, if the whitelist is disabled, the absence of msg.value checks can lead to an increased number of spam requests, further congesting the network.
Vulnerability Details
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;
req.header = Protocol.requestHeaderV1(ctype, useAutoBurn, false);
req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids);
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);
l1ToL2Messages()[msgHash] = msg.value + 1;
return (msgHash, nonce);
}
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2() is used to call the Starknet core method, with the purpose of sending a message to L2, awaiting further processing by bridge.cairo#[l1_handler] after capture. The ETH transferred by msg.value includes the gas and some other fees required for the L2 contract call. We can see that the StarknetMessaging::sendMessageToL2() source code only contains the check of the upper and lower limits of msg.value.
At this point, the Bridge::depositTokens() method may encounter a common problem. Insufficient msg.value causes ETH and NFT to be locked in the contract. The administrator needs to call the Bridge::startRequestCancellation() -> StarknetMessaging::startL1ToL2MessageCancellation() function to cancel the erroneous transaction, which sets a 5-day waiting period. After waiting for 5 days, the user calls the Bridge::cancelRequest() -> StarknetMessaging::cancelL1ToL2Message() function to cancel.
function startRequestCancellation(
uint256[] memory payload,
uint256 nonce
) external onlyOwner {
@> IStarknetMessaging(_starknetCoreAddress).startL1ToL2MessageCancellation(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
emit CancelRequestStarted(req.hash, block.timestamp);
}
function cancelRequest(
uint256[] memory payload,
uint256 nonce
) external {
@> IStarknetMessaging(_starknetCoreAddress).cancelL1ToL2Message(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
_cancelRequest(req);
emit CancelRequestCompleted(req.hash, block.timestamp);
}
In summary, if msg.value is insufficient, the user's NFT will be locked for 5 days, and the user will also lose the ETH sent in the operation. Additionally, if the whitelist is disabled, the lack of msg.value checks can lead to an increase in spam requests, further congesting the network and increasing the cost for legitimate users.
Reference Documentation
L1-L2 Messaging
function sendMessageFelt(
uint256 contractAddress,
uint256 selector,
uint256 myFelt
)
external
payable
{
uint256[] memory payload = new uint256[](1);
payload[0] = myFelt;
@>
_snMessaging.sendMessageToL2{value: msg.value}(
contractAddress,
selector,
payload
);
}
It's important to note that we have {value: msg.value}. In fact, the minimum value we've to send here is 20k wei, due to the fact that the StarknetMessaging contract will register the hash of our message in the storage of Ethereum.
In addition to those 20k wei, since the L1HandlerTransaction executed by the sequencer is not tied to any account (the message originates from L1), you must also ensure that you pay enough fees on L1 for your message to be deserialized and processed on L2.
Poc
Please add the code to test/Bridge.t.sol and execute it:
function test_depositTokenERC721_with_1_eth() public {
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
IStarklane(bridge).depositTokens{value: 1}(
salt,
address(erc721C1),
to,
ids,
false
);
vm.stopPrank();
}
Code Snippet
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/ethereum/src/Bridge.sol#L78-L144
Impact
Bridge::depositTokens() lacks the evaluation and check of msg.value, which may result in the loss of ETH and the NFT being locked in the contract. Furthermore, disabling the whitelist without this check can lead to an increase in spam requests, causing network congestion and increasing costs for legitimate users.
Tools Used
Manual Review
Recommendations
We should consider adding implementation for evaluating and checking msg.value
For example:
// Dummy implementation for payload size fee calculation
+ uint256 constant PAYLOAD_SIZE_FEE = 0.0001 ether;
+ uint256 constant FIXED_COST = 5000; // Quoting the documentation `incurs a fixed cost of 5,000 gas`
+ function estimateMessageSendingFee(uint256[] calldata payload) internal view returns (uint256) {
// Get the current network basic fee (need to implement actual logic)
+ uint256 baseFee = getCurrentNetworkBaseFee();
// Calculate the cost based on the message parameter size
+ uint256 payloadSizeFee = payload.length * PAYLOAD_SIZE_FEE; // Calculate fees based on payload size
// Total cost
+ return baseFee + payloadSizeFee + FIXED_COST; // Including fixed costs
+ }
// Example function: Get the current network basic fee (need to implement actual logic)
+ function getCurrentNetworkBaseFee() internal view returns (uint256) {
// You can use the on-chain API or third-party tools to obtain the current base fee
+ return 0.001 ether; // Example return value
+ }
function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// Existing checks and logic...
// Adding checks
+ uint256 estimatedGasFee = estimateMessageSendingFee(payload);
+ require(msg.value >= estimatedGasFee, "Insufficient gas fee");
// Existing logic: Send message to L2...
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}
This change ensures that the msg.value is sufficient to cover the gas fees and prevents the contract from being spammed with low-cost requests.
