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

Low Issues

bauchibred

Low Issues

L-01 ERC1155 Transfer Restriction Bypass in L2 Withdrawal

Summary

The withdraw_auto_from_l1 function in the L2 bridge contract lacks a check for ERC1155 tokens, allowing the transfer of unsupported token types. This contrasts with the L1 depositTokens function, which explicitly rejects ERC1155 tokens.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L129-L181

fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
ensure_is_enabled(@self);
assert(self.bridge_l1_address.read().into() == from_address,
'Invalid L1 msg sender');
​
// TODO: recompute HASH to ensure data are not altered.
// TODO: Validate all fields the request (cf. FSM).
​
let collection_l2 = ensure_erc721_deployment(ref self, @req);
​
let _ctype = collection_type_from_header(req.header);
// TODO: check CollectionType to support ERC1155 + metadata.
​
// ... (rest of the function)
}

Impact

This check can easily be sidestepped via the sequencer directly on the L2, potentially leading to unexpected behavior and security risks in handling unsupported token types.

Tools Used

Manual review

Recommendations

Implement a check for ERC1155 tokens in the withdraw_auto_from_l1 function.

L-02 Missing Maximum Payload Length Check in L2 Withdrawal

Summary

The L2 withdraw_auto_from_l1 function does not implement any limit on the number of tokens it processes, unlike the L1 depositTokens function which enforces a maximum payload length of 300.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L128-L182

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
// ... (code omitted for brevity)
​
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
// ... (processing each token)
i += 1;
};
​
// ... (rest of the function)
}

Impact

This discrepancy could lead to potential out-of-gas errors or excessive resource consumption on the L2 side, causing transaction failures since this can be accessed directly via the sequencer.

Tools Used

Manual review

Recommendations

Implement a maximum token limit in the withdraw_auto_from_l1 function:

const MAX_TOKENS: usize = 300; // Or an appropriate limit
​
assert(req.ids.len() <= MAX_TOKENS, 'Too many tokens');

L-03 Lack of Replay Protection During Cross-chain Withdrawals

Summary

The withdraw_auto_from_l1 function lacks a mechanism to prevent replay attacks, similar to the depositTokens() on L1.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L128-L182

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
ensure_is_enabled(@self);
assert(self.bridge_l1_address.read().into() == from_address,
'Invalid L1 msg sender');
​
// TODO: recompute HASH to ensure data are not altered.
// TODO: Validate all fields the request (cf. FSM).
​
// ... (rest of the function)
}

Impact

Without this protection, malicious actors could replay the same withdrawal request multiple times, leading to unauthorized token minting or transfers. This vulnerability could significantly compromise the integrity of the bridge and the token ecosystem.

Tools Used

Manual review

Recommendations

Implement a nonce system to track processed requests.

L-04 Inconsistent Metadata Handling Across Chains

Summary

The Solidity implementation on Ethereum (L1) distinguishes between ERC721 and ERC1155 tokens when fetching metadata, while the Cairo implementation on StarkNet (L2) assumes all tokens are ERC721, without checking the token type.

Vulnerability Details

Take a look at the Solidity implementation here and the Cairo implementation here.

Impact

This discrepancy can lead to incorrect or missing metadata for ERC1155 tokens, affecting their functionality and potentially causing the bridge to fail or behave unexpectedly when handling these tokens. It could result in loss of metadata, incorrect token information on L2, and a degraded user experience.

Tools Used

Manual review

Recommendations

Add logic in the Cairo deposit_tokens function to detect the token type (ERC721 vs. ERC1155) and handle metadata fetching accordingly.

L-05 L1L2CollectionMappingUpdated is Currently Unused On The L2

Summary

The L1L2CollectionMappingUpdated event is defined but not used in the set_l1_l2_collection_mapping function.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L359-L364

fn set_l1_l2_collection_mapping(ref self: ContractState, collection_l1: EthAddress, collection_l2: ContractAddress) {
ensure_is_admin(@self);
self.l1_to_l2_addresses.write(collection_l1, collection_l2);
self.l2_to_l1_addresses.write(collection_l2, collection_l1);
}

Impact

Low, since StarkNet takes events seriously.

Tools Used

Manual review

Recommendations

Attach the event to set_l1_l2_collection_mapping().#

L-06 Ether can be forcibly sent to the contract despite reverting receive() function

Summary

The contract attempts to prevent receiving Ether by implementing a receive() function that reverts. However, this protection can be bypassed, allowing Ether to be forcibly sent to the contract.

Vulnerability Details

See https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/UUPSProxied.sol#L49-L57

/**
@notice Ensures no ether is received without a function call.
*/
receive()
external
payable
{
revert NotPayableError();
}

After conversations with teh sponsors they had indicated that this was implemented as a subtle invariant since they never want to have any eth in their contracts and have no functionality to remove them if need be.

Now while the contract implements a receive() function that reverts to prevent receiving Ether, there are two ways to bypass this protection:

  1. Using selfdestruct(): A malicious contract can use selfdestruct() to forcibly send its balance to any address, including this contract.

  2. Block mining rewards: If the contract's address is set as the recipient of block mining rewards, it will receive Ether regardless of the receive() function.

These methods can force Ether into the contract without triggering the receive() function, potentially disrupting the contract's intended behavior if it assumes it will never hold Ether.

Impact

The impact is low because:

  1. It doesn't directly lead to loss of funds or critical vulnerabilities.

  2. The contract doesn't seem to rely on the assumption of having zero balance for its core functionality.

However, it breaks the subtle invariant of the contract/system assumption that the contract will never hold Ether.

Tools Used

Manual review

Recommendations

Instead of relying solely on preventing Ether reception, implement a way to handle unexpected Ether in the contract:

function withdraw() external onlyOwner {
uint256 balance = address(this).balance;
if (balance > 0) {
(bool success, ) = msg.sender.call{value: balance}("");
require(success, "Transfer failed");
}
}

L-07 Erc1155 tokens are DOS'd from being deposited on the cairo bridge

Summary

ON the cairo side we can never process erc 1155 deposits

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L229-L293

fn deposit_tokens(
ref self: ContractState,
salt: felt252,
collection_l2: ContractAddress,
owner_l1: EthAddress,
token_ids: Span<u256>,
use_withdraw_auto: bool,
use_deposit_burn_auto: bool,
) {
ensure_is_enabled(@self);
assert(!self.bridge_l1_address.read().is_zero(), 'Bridge is not open');
​
// we may have the "from" into the params, to allow an operator
// to deposit token for a user.
// This can open more possibilities.
// Because anyway, a token can't be moved if the owner of the token
// is not giving approval to the bridge.
let from = starknet::get_caller_address();
​
assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted');
​
// TODO: add support for 1155 and check the detected interface on the collection.
let ctype = CollectionType::ERC721;
​
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids));
let (name, symbol, base_uri, uris) = match erc721_metadata {
Option::Some(data) => (data.name, data.symbol, data.base_uri, data.uris),
Option::None => ("", "", "", array![].span())
};
​
escrow_deposit_tokens(ref self, collection_l2, from, token_ids);
​
let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
​
let req = Request {
header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
collection_l1,
collection_l2,
owner_l1,
owner_l2: from,
name,
symbol,
base_uri,
ids: token_ids,
values: array![].span(),
uris,
new_owners: array![].span(),
};
​
let mut buf: Array<felt252> = array![];
req.serialize(ref buf);
​
starknet::send_message_to_l1_syscall(self.bridge_l1_address.read().into(), buf.span(),)
.unwrap_syscall();
​
self
.emit(
DepositRequestInitiated {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
}
);
}

This function is used to Deposit tokens to be bridged on the L1, this is in stock to already be used however any erc1155 deposits would not be correctly processed due to us not getting it's metadata.

Impact

ERC 1155 deposits onto the l1 from the cairo bridge is not supported.

Recommendations

Support these deposits

L-08 It's currently impossibe to the retrieve the metadata for some tokena

Summary

ERC 1155 token's metadata can never be retireved

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/TokenUtil.sol#L113-L137

function erc1155Metadata(
address collection
)
internal
view
returns (string memory)
{
return "";
/* bool supportsMetadata = ERC165Checker.supportsInterface( */
/* collection, */
/* type(IERC1155MetadataURI).interfaceId */
/* ); */
/* if (!supportsMetadata) { */
/* return ""; */
/* } else { */
/* // TODO: ideally, we should get one URI of a token, */
/* // and extract the constant part? All tokens are supposed to have */
/* // the same base address, only replacing the `{id}`. If it's the case, */
/* // we can take the uri of the first token, and return it... */
/* //return IERC1155MetadataURI(collection).uri(WhichTokenId?); */
/* return "TODO"; */
/* } */
​
}

This is from the tokensUtil contract and is the helper function that is to help retrieve the metadata from ERC1155, however it currently hardcodes the return value and instead returns "".

Impact

ERC 1155 token's metadata can never be retireved

Tools Used

Manual review

Recommendations

Uncomment the lines

L-09 Lack of Offset and Array Size Checks in Serialization Functions

Summary

The current implementation lacks proper offset and array size checks during serialization operations, which could lead to buffer overflows or out-of-bounds access.

Vulnerability Details

Take a look at the serialization functions, such as uint256ArraySerialize and cairoStringSerialize. These functions currently do not perform checks to ensure that the offset and array sizes are within the bounds of the provided buffer.

For example, in uint256ArraySerialize:

function uint256ArraySerialize(
uint256[] memory arr,
uint256[] memory buf,
uint256 offset
)
internal
pure
returns (uint256)
{
uint256 _offset = offset;
​
// Arrays always have their length first in Cairo.
buf[_offset] = arr.length;
_offset++;
​
for (uint256 i = 0; i < arr.length; i++) {
_offset += uint256Serialize(arr[i], buf, _offset);
}
​
return _offset - offset;
}

There are no checks to ensure that _offset remains within the bounds of buf.

Impact

This vulnerability could lead to:

  1. Buffer overflows, potentially overwriting unintended memory areas.

  2. Out-of-bounds access, causing runtime errors or unexpected behavior.

  3. Potential security vulnerabilities if an attacker can manipulate input data to exploit these issues.

Recommendations

  1. Implement bounds checking for all offset operations.

  2. Add array size validation to ensure the buffer can accommodate the serialized data.

  3. Use assert statements for these checks, as suggested in the TODOs.

Example implementation:

function uint256ArraySerialize(
uint256[] memory arr,
uint256[] memory buf,
uint256 offset
)
internal
pure
returns (uint256)
{
uint256 _offset = offset;
assert(_offset < buf.length);
assert(_offset + 1 + (arr.length * 2) <= buf.length);
​
buf[_offset] = arr.length;
_offset++;
​
for (uint256 i = 0; i < arr.length; i++) {
_offset += uint256Serialize(arr[i], buf, _offset);
}
​
return _offset - offset;
}

L-10 Lack of UTF-8 Support in Cairo Short String Implementation

Summary

The current implementation of Cairo Short Strings assumes ASCII encoding, limiting the range of characters that can be represented and potentially causing issues with non-ASCII text.

Vulnerability Details

Take a look at the cairoStringPack and cairoStringUnpack functions. These functions currently treat each character as a single byte, which is not compatible with UTF-8 encoding where characters can occupy 1 to 4 bytes.

function cairoStringPack(
string memory str
)
internal
pure
returns (uint256[] memory)
{
bytes memory strBytes = bytes(str);
uint256 dataLen = strBytes.length / CAIRO_STR_LEN;
uint256 pendingLen = strBytes.length % CAIRO_STR_LEN;
// ...
}

Impact

  1. Inability to properly handle non-ASCII characters, leading to data loss or corruption when processing strings containing such characters.

  2. Potential security vulnerabilities if an attacker can exploit this limitation to bypass input validation or cause unexpected behavior.

  3. Incompatibility with systems or data sources that use UTF-8 encoding.

Recommendations

  1. Implement UTF-8 support in the Cairo Short String functions.

  2. Update the packing and unpacking logic to handle multi-byte characters correctly.

  3. Adjust the CAIRO_STR_LEN constant to account for the maximum number of UTF-8 bytes that can fit in a Cairo Short String, rather than the number of characters.

Example conceptual implementation (note that this would require significant changes to the existing code):

function cairoStringPack(
string memory str
)
internal
pure
returns (uint256[] memory)
{
bytes memory strBytes = bytes(str);
uint256[] memory packedData = new uint256[]((strBytes.length + 31) / 32 + 2);
uint256 index = 1;
uint256 byteIndex = 0;
uint256 currentWord = 0;
uint256 bitsUsed = 0;
​
while (byteIndex < strBytes.length) {
uint8 byteVal = uint8(strBytes[byteIndex]);
currentWord = (currentWord << 8) | byteVal;
bitsUsed += 8;
if (bitsUsed == 248 || byteIndex == strBytes.length - 1) {
packedData[index] = currentWord << (256 - bitsUsed);
index++;
currentWord = 0;
bitsUsed = 0;
}
byteIndex++;
}
​
packedData[1] = index - 1; // Number of packed words
packedData[index] = byteIndex; // Total number of bytes
​
return packedData;
}

This implementation would require corresponding changes to cairoStringUnpack and other related functions.

Updates

Lead Judging Commences

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

invalid-UTF8-not-supported

Serialization and deserialization are made directly on bytes, no data are lost during the transfer.

Support

FAQs

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