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

There is no check to ensure that msg.value is above minimum or below max fees in depositTokens

helium

Summary

As i already pointed in another report, to,avoid user NFT being stuck inside the SnMessaging contract, we should ensure that user send enough msg.value to cover:

  1. Registration of the msgHash on ethereum storage by snMessaging.

  2. The call of the L1HandlerTransaction by the sequencer on Starknet.

Recommended mitigation greatly address this issue, however there is a MAX_L1_MSG_FEE constant inside SnMessaging contract, to ensure msg.value sent does not exceed this constant see: https://github.com/starkware-libs/cairo-lang/blob/0e4dab8a6065d80d1c726394f5d9d23cb451706a/src/starkware/starknet/solidity/StarknetMessaging.sol#L29-L33, And the Bridge.cairo contract fails to check this inside depositTokens() function.

Vulnerability Details

Because there is no check to ensure msg.value does not exceed the MAX_L1_MSG_FEE, if an user send a transaction with a msg.value that exceed the max fee, the transaction will revert.

To illustrate i wrote a test that revert because we send a value exceeding the max fee. It erevert with MAX_L1_MSG_FEE_EXCEEDED. Copy and paste inside Bridge.t.sol and run it with: forge test --mt test_MAXFEEEXCEEDED -vv

function test_MAXFEEEXCEEDED() 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);
// We send a transaction with an arbitrarily high value so the tx revert as expected
// It will revert with MAX_L1_MSG_FEE_EXCEEDED
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
IStarklane(bridge).depositTokens{value: 100 ether}(
salt,
address(erc721C1),
to,
ids,
false
);
​
vm.stopPrank();
}

Impact

Function call will revert when the MAX_L1_MSG_FEE is exceeded

Tools Used

Manual review

Recommendations

There is a getMaxL1MsgFee() function inside SnMessaging, call it and get the max fee, then check the msg.value against it to ensure it does not exceed before sending the tx. See: https://github.com/starkware-libs/cairo-lang/blob/0e4dab8a6065d80d1c726394f5d9d23cb451706a/src/starkware/starknet/solidity/StarknetMessaging.sol#L31

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.