NFTBridge
60,000 USDC
View results
Submission Details
Severity: medium
Valid

No check for low gas on L1 deposit

Summary

Check for the msg.value provided to Bridge::depositTokens is missing and users can perform free bridging since L2 sequencer picking every transaction no matter how much gas is used.

Also there is a note in the Cairo docs saying that minimum 20k wei must be forwarded in order to bridge successfully:

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.

Vulnerability Details

Not providing enough gas fees to the depositTokens will lead to either users bridging for free or when fees are enforced from the sequencer transactions providing less than 20k wei to fail.

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

The consequence of this actions are that NFTs will not be able to be bridged and will be locked in the bridge until the transactions is cancelled, but cancellation consists of two steps and the first one - Bridge::startRequestCancellation have onlyOwner modifier.

Not everyone will be able to cancel his failed transaction until the admin doesn’t cancel them, and if there are a lot of failing transaction that should be canceled this can lead to prolonged periods of locked tokens for the users.

Bridge.sol

function startRequestCancellation(
uint256[] memory payload,
uint256 nonce
) external onlyOwner {

Additionally, since depositTokens allows users to bridge multiple tokens, the gas to be provided must be scaled based on the number of NFTs deposited.

Impact

Prolonged locked NFTs due to not enforcing minimum gas forwarded to depositTokens.

Tools Used

Manual review

Recommendations

Introduce a minimum gas variable with an admin setter and scale it based on the amount of NFTs deposited. This value can be obtained by testing on a testnet how much gas will cost for 1 NFT and then scaled. Also add 20k wei as a buffer as suggested in the docs.

Something like this:

+ require(msg.value >= (minimumGasForBridge * ids.length) + 20000);
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.