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

Wrong conversion of felt252 to ByteArray for 31-byte data

fyamf

Summary

The function try_into(self: felt252) does not convert correctly a 31-byte felt252 to ByteArray. This gives wrong result during depositing on L2. The PoC clearly shows the difference.

Vulnerability Details

Function try_into(self: felt252) which converts felt252 to ByteArray is not creating a standard ByteArray. As explained in the following link a ByteArray is actually a struct with the following members:

  1. data: Array<felt252>: Contains 31-byte chunks of the byte array. Each felt252 value has exactly 31 bytes. If the number of bytes in the byte array is less than 31, then this array is empty.

  2. pending_word: felt252: The bytes that remain after filling the data array with full 31-byte chunks. The pending word consists of at most 30 bytes.

  3. pending_word_len: usize: The number of bytes in pending_word.
    https://docs.starknet.io/architecture-and-concepts/smart-contracts/serialization-of-cairo-types/#serialization_of_byte_arrays

As it clearly states pending_word consists of at most 30 bytes. But, the way the function try_into() is implemented, is not compatible with this structure.

For example, giving a 31-byte felt252 'qwertyuiopasdfghjklzxcvbnm12345' as parameter to the function try_into(self: felt252) returns the following data:

  • data.is_empty(): true

  • pending_word: 200477761143354947435595494819051448281747198402755701270381458527154811957

  • pending_word_len: 31

While a ByteArray equal to "qwertyuiopasdfghjklzxcvbnm12345" has the following specifications:

  • data.is_empty(): false

  • pending_word: 0

  • pending_word_len: 0

The root cause of this issue is that when a 31-byte flet252 is forwarded as a parameter to this function, it should be considered exactly as a chunk of 31-byte, so there should be left nothing for the pending word. But, the function try_into(self: felt252) always assumes that data is empty, and stores the forwarded parameter in the pending word.

fn try_into(self: felt252) -> Option<ByteArray> {
let mut res: ByteArray = Default::default();
res.pending_word = self;
let mut length = 0;
let mut data: u256 = self.into();
loop {
if data == 0 {
break;
}
data /= 0x100;
length += 1;
};
res.pending_word_len = length;
Option::Some(res)
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/byte_array_extra.cairo#L4

The impact is as follows:

When depositing on L2, during calculation of erc721_metadata, the collection contract is called to get the token uri.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L266
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/collection_manager.cairo#L50

The returned data from the collection is processed by invoking try_into(self: Span<felt252>).

match starknet::call_contract_syscall(
collection_address,
token_uri_selector,
calldata,
) {
Result::Ok(span) => span.try_into(),
Result::Err(_e) => {
match starknet::call_contract_syscall(
collection_address, tokenURI_selector, calldata,
) {
Result::Ok(span) => span.try_into(),
Result::Err(_e) => {
Option::None
}
}
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/collection_manager.cairo#L112-L117

If the length of self is equal to 1, the function try_into(self: felt252) would be called.

impl SpanFeltTryIntoByteArray of TryInto<Span<felt252>, ByteArray> {
fn try_into(self: Span<felt252>) -> Option<ByteArray> {
if self.len() == 0_usize {
Option::None(())
} else if self.len() == 1_usize {
(*self[0]).try_into()
} else {
let mut self = self.clone();
Serde::deserialize(ref self)
}
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/byte_array_extra.cairo#L27

Here is where the problem shows up. As explained before, if self is 31-byte felt252, then the returned ByteArray will have incorrect format.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/byte_array_extra.cairo#L4

PoC

In the following test, tow variables a and b are equal to 31-byte qwertyuiopasdfghjklzxcvbnm12345, where a is felt252, and b is ByteArray. It is expected that when a is converted from flet252 to ByteArray, the specification of resulted ByteArray is equal to b. But this is not the case for 31-byte input.

#[test]
fn input_31_byte() {
let a: felt252 = 'qwertyuiopasdfghjklzxcvbnm12345';
​
let convert: Option<ByteArray> = a.try_into();
match convert {
Option::Some(e) => {
println!("\nThe output of the function:");
println!("{:?}", e.data.is_empty());
println!("{:?}", e.pending_word);
println!("{:?}", e.pending_word_len);
},
Option::None => panic!("Should not be None")
}
​
let b: ByteArray = "qwertyuiopasdfghjklzxcvbnm12345";
​
println!("\nCorrect output:");
println!("{:?}", b.data.is_empty());
println!("{:?}", b.pending_word);
println!("{:?}", b.pending_word_len);
}

The output log is:

The output of the function:
true
200477761143354947435595494819051448281747198402755701270381458527154811957
31
​
Correct output:
false
0
0

Running the same test for 10-byte input shows that the outputs are equal:

#[test]
fn input_10_byte() {
let a: felt252 = 'qwertyuiop';
​
let convert: Option<ByteArray> = a.try_into();
match convert {
Option::Some(e) => {
println!("\nThe output of the function:");
println!("{:?}", e.data.is_empty());
println!("{:?}", e.pending_word);
println!("{:?}", e.pending_word_len);
},
Option::None => panic!("Should not be None")
}
​
let b: ByteArray = "qwertyuiop";
​
println!("\nCorrect output:");
println!("{:?}", b.data.is_empty());
println!("{:?}", b.pending_word);
println!("{:?}", b.pending_word_len);
}

The output log is:

The output of the function:
true
535829885142251531235184
10
​
Correct output:
true
535829885142251531235184
10

Impact

  • Wrong conversion of felt252 to ByteArray during depositing on L2.

Tools Used

Recommendations

It is recommended that when the forwarded parameter has length 31-byte, it should set pending_word = '' and pending_word_len = 0, and the forwarded parameter (self) should be assigned to data.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid-31bytes-felt252-try_into-BytesArray-incorrect-decoding

Great catch ! Unfortunatelly there is no impact... :/ I simulated the end of the calling functions and it seems that the "span" conversion handle that case. Here is my PoC: ``` fn input_31_byte() { let a: felt252 = 'qwertyuiopasdfghjklzxcvbnm12345'; let mut out_uris = array![]; let convert: Option<ByteArray> = a.try_into(); match convert { Option::Some(e) => { out_uris.append(e); }, Option::None => panic!("Should not be None") } let span = out_uris.span(); println!("Real data used in the calling function {:?}", span[0]); let b: ByteArray = "qwertyuiopasdfghjklzxcvbnm12345"; let mut out_uris_2 = array![]; out_uris_2.append(b); println!("Real data used in the calling function {:?}", out_uris_2.span()[0]); } ```

Support

FAQs

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