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

There is No `msg.value` check in `depositTokens`, causing potential token stuck

jesjupyter

Summary

In the Starklane::depositTokens function, users are required to send ether along with their transaction. According to the documentation, to simplify interactions between Layer 1 (L1) and Layer 2 (L2), L1 → L2 messages are payable on L1 by sending ETH with the call to the payable function sendMessageToL2 on the Starknet Core Contract. However, the current implementation lacks a fee estimation function and does not validate the msg.value parameter. properly.

Consequences:

• Users cannot accurately estimate the required fee, leading to uncertainty.

• If a user sends less ether than required, the cross-chain transaction may not proceed on L2, or the hash may fail to be written back, causing unintended consequences. For example, users who do not receive their NFTs may have to wait an additional 5 days to cancel the action, causing their tokens to be temporarily stuck.

• If a user sends more ether than necessary, the excess will not be refunded, leading to a loss of funds.

Vulnerability Details

The Starklane::depositTokens function requires users to send ether, which is used to call sendMessageToL2 on the Starknet Core Contract:

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
...
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
...
}

In the L1 <==> L2 cross-chain mechanism, this ether is used to pay for the transaction on L2, as outlined in the Starknet documentation. However, there is no functionality to determine the correct amount of ether to send, nor is there a check on msg.value to ensure the ether sent is within an acceptable range. The StarknetMessaging::sendMessageToL2 function does not calculate or validate the fee either.

Per the documentation, there is a minimum fee required for the message to be fully processed:

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.

The following PoC demonstrates the vulnerability:

function testERC721LowValue() 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();
​
// TODO: test event emission to verify the request.
}

This code demonstrates that a deposit still succeeds even when only 1 wei is transferred, and events are emitted:

LogMessageToL2(fromAddress: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], toAddress: 1, selector: 2, payload: [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 0, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], nonce: 0, fee: 1)

The PoC was executed on forked-ETH with forge test --mt testERC721LowValue2 -vvvv --evm-version shanghai. It confirms that the event is emitted successfully, even with an insufficient fee.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;
​
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC1155/IERC1155.sol";
​
import "forge-std/Test.sol";
import "../src/IStarklane.sol";
import "../src/IStarklaneEvent.sol";
​
import "../src/Bridge.sol";
import "../src/sn/Cairo.sol";
import "../src/sn/StarknetMessagingLocal.sol";
import "../src/sn/IStarknetMessagingLocal.sol";
import "../src/token/Deployer.sol";
import "../src/token/TokenUtil.sol";
​
import "./utils/Users.sol";
import "./token/ERC721MintFree.sol";
import "./token/IERC721MintRangeFree.sol";
​
/**
@title Bridge testing.
*/
contract BridgeBugTest is Test, IStarklaneEvent {
​
address bridge;
address erc721C1;
address erc1155C1;
address snCore;
​
address payable internal alice;
address payable internal bob;
​
//
function setUp() public {
// select fork to use mainnet
vm.createSelectFork("https://eth.llamarpc.com",20_584_798);
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
​
// "Regular" collections, not controlled by the bridge.
erc721C1 = address(new ERC721MintFree("name 1", "S1"));
​
snCore = address(new StarknetMessagingLocal());
​
address impl = address(new Starklane());
bytes memory dataInit = abi.encodeWithSelector(
Starklane.initialize.selector,
abi.encode(
address(this),
0xc662c410C0ECf747543f5bA90660f6ABeBD9C8c4,
0x1,
0x2
)
);
​
bridge = address(new ERC1967Proxy(impl, dataInit));
IStarklane(bridge).enableBridge(true);
}
​
function testERC721LowValue2() 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();
​
// TODO: test event emission to verify the request.
}
}

Impact

As a result:

• Users cannot estimate the fee they should pay due to the lack of functionality.

• If a user sends less than the required amount, the cross-chain transaction might not proceed on L2, leading to unintended consequences like temporary token lockups. Users who do not receive their NFTs might have to wait an additional 5 days to cancel the action.

• If a user overpays, the excess fee will not be refunded, resulting in a loss of funds.

Tools Used

Manual review, Foundry

Recommendations

• Implement a minimum fee validation to try to prevent temporary token lockup.

• Provide a query function (or update the fee estimation process) to help users more accurately determine the required fee for transactions.

Updates

Lead Judging Commences

n0kto Lead Judge 9 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

Appeal created

sammy Auditor
9 months ago
nirohgo Auditor
9 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 9 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.