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

`depositTokens` allow `ownerL2` to be zero address, which leads to locking of tokens in escrow

0xtheblackpanther

Github

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

Summary

The depositTokens function in the provided contract allows users to deposit tokens into escrow and initiates the transfer to L2. However, there is a potential issue when the ownerL2 address is set to zero.

Impact

If the ownerL2 address is zero, it could lead to a situation where tokens are locked in escrow on L1 without being transferred to the intended recipient on L2. This could cause user funds to be inaccessible, leading to potential financial losses and a loss of trust in the system.

As it is user input so likelihood is very low and can be considred user mistake, but the impact is high because user tokens gets lock in the escrow. So the final severity will be low.

Proof of Concept

The depositTokens function includes a parameter ownerL2, which represents the new owner's address on StarkNet. The function checks if ownerL2 is a valid felt252 value, but it does not explicitly check if ownerL2 is non-zero. Here’s a simplified version of the critical part of the function:

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();
}
​
// Other checks and logic...
​
req.ownerL2 = ownerL2;
​
// Serialize and send the message to L2
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);
}

Recommendation

To mitigate this issue, add an explicit check to ensure that ownerL2 is not zero. Here is a potential solution:

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
if (!Cairo.isFelt252(snaddress.unwrap(ownerL2)) || snaddress.unwrap(ownerL2) == 0) {
revert CairoWrapError();
}
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
// Other checks and logic...
​
req.ownerL2 = ownerL2;
​
// Serialize and send the message to L2
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);
}

By ensuring ownerL2 is non-zero, we can prevent tokens from being locked in escrow and ensure they are transferred to a valid address on StarkNet.

Updates

Lead Judging Commences

n0kto Lead Judge 12 months 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

0xtheblackpanther Submitter
12 months ago
n0kto Lead Judge
11 months ago
n0kto Lead Judge 11 months 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.