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

`Bridge::depositTokens()` does not check for minimum required fee

decap

Summary

Bridge::depositTokens() does not check for minimum required fee

Vulnerability Details

Minimum fee is required in order message from L1 to be deserialized and processed on L2. According to docs https://book.cairo-lang.org/ch16-04-L1-L2-messaging.html, minimum required fee is 20k wei + some additional value.

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.

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

StarknetMessaging::sendMessageToL2checks only if msg.value > 0.

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/lib/starknet/StarknetMessaging.sol#L126

Impact

Medium, message cannot be processed on L2 when not enough fees sent

Tools Used

Manual review

Recommendations

Add require check in Bridge::depositTokens for minimum fee.

You can profile the gas consumption using starkli or snforge to estimate the cost of your message execution.

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();
}
​
+ require(msg.value > MIN_REQUIRED_FEE, "Not enough fee sent");
​
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);
}
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.