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

Risk of invalid L1 -> L2 address conversion

0xnbvc

Vulnerability Details

Addresses on Layer 2 (Starknet) are of type felt252 while on the Layer 1 (Ethereum) are of type uint160. It's important that the matching between the different address versions is preserved correctly.
Particularly during cross-layer messaging, Solidity addresses are given as uint256 which is greater than felt252 and this may cause issues in the matching.

A good way to avoid this type of issues is to ensure that when sending a message from L1 to L2, all inputs are validated properly: address should be lower than the Finite Field order p used on the Layer 2.

This is correctly done in Bridge::depositTokens():

Bridge.sol#L88-L90

-> if (!Cairo.isFelt252(snaddress.unwrap(ownerL2))) {
revert CairoWrapError();
}

However, this is not done in Bridge::setL1L2CollectionMapping() / CollectionManager::_setL1L2AddressMapping():

CollectionManager.sol#L151-L165

function _setL1L2AddressMapping(
address collectionL1,
-> snaddress collectionL2,
bool force
)
internal
{
if (((snaddress.unwrap(_l1ToL2Addresses[collectionL1]) == 0) && (_l2ToL1Addresses[collectionL2] == address(0)))
|| (force == true)) {
-> _l1ToL2Addresses[collectionL1] = collectionL2;
_l2ToL1Addresses[collectionL2] = collectionL1;
} else {
revert CollectionMappingAlreadySet();
}
}

This _l1ToL2Addresses mapping is then used in Bridge::depositTokens() to build the request to be passed to L2. Thus, if on L1 it builds a message with values above the maximum felt252, the message will be stuck and never consumed on L2.

Impact

This can cause severe disruption to the protocol's functionality, as all users' calls to Bridge::depositTokens() will be stuck and never consumed on L2. The admin will have to call Bridge::startRequestCancellation(), wait 5 days, and then call Bridge::cancelRequest() for each transaction to recover the NFTs.

Recommendations

Add the same check used in Bridge::depositTokens() to CollectionManager::_setL1L2AddressMapping():

function _setL1L2AddressMapping(
address collectionL1,
snaddress collectionL2,
bool force
)
internal
{
+ if (!Cairo.isFelt252(snaddress.unwrap(collectionL2))) {
+ revert CairoWrapError();
+ }
​
if (((snaddress.unwrap(_l1ToL2Addresses[collectionL1]) == 0) && (_l2ToL1Addresses[collectionL2] == address(0)))
|| (force == true)) {
_l1ToL2Addresses[collectionL1] = collectionL2;
_l2ToL1Addresses[collectionL2] = collectionL1;
} else {
revert CollectionMappingAlreadySet();
}
}
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.

Appeal created

0xnbvc Submitter
about 1 year ago
n0kto Lead Judge
about 1 year ago
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.

Give us feedback!