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

Incorrect implementation of `felt252.try_into()`

n4nika

Summary

The felt252.try_into() implementation in byte_array_extra.cairo is incorrect leading to felts being serialized into a format other than specified in cairo.

Vulnerability Details

What the function does is take a single felt252 and convert it to a ByteArray. If we look at the documentation of serialization in cairo (https://docs.starknet.io/architecture-and-concepts/smart-contracts/serialization-of-cairo-types/) we can look at the section Serialization of byte arrays:

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.

Most importantly note that The pending word consists of at most 30 bytes.

Now let's look at the implementation in byte_array_extra.cairo:

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)
}

We see that a new ByteArray is created and self (felt252) is assigned to its pending_word.
Now the max value for a felt252 is 3618502788666131213697322783095070105623107215331596699973092056135872020480, which contains 252 bits. 252 bits can represent 31.5 bytes (31.5 * 8 = 252).

Here we see the problem which is that if we have a very big felt252, exceeding 30 bits, the function does not provide correct results anymore as the pending_word will contain 31 or even 32 bytes (the half byte at the top is taken a a full byte with the first 4 bits being zero).

Impact

This means that felts will be converted to ByteArrays which are technically incorrectly serialized.

Proof of Concept

In order to show the incorrect serialization please add the following test to byte_array_extra.cairo and execute it with snforge test from_felt252_wrong_serialization.

#[test]
fn from_felt252_wrong_serialization() {
let a = 3618502788666131213697322783095070105623107215331596699973092056135872020480; // MAX value
println!("before: {:?}", a);
let b: Option<ByteArray> = a.try_into();
match b {
Option::Some(result) => {
println!("result: {:?}", result);
​
println!("result.data.len(): {:?}", result.data.len());
println!("result.pending_word_len: {:?}", result.pending_word_len);
println!("result.len(): {:?}", result.len());
},
Option::None => panic!("Should not be None")
}
}

Tools Used

Manual review

Recommended Mitigation

In order for try_into to correctly serialize felts to byte arrays, the function would need to check whether the felt contains more than 30 bytes and if so, the felt needs to be split into data and pending_word in the resulting byte array.

Updates

Lead Judging Commences

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