minor(ampd): update signers hash for multiversx after contracts audit. #707
Conversation
ampd/src/mvx/mod.rs
Outdated
| fn usize_to_u32(value: usize) -> Vec<u8> { | ||
| (value as u32).to_be_bytes().to_vec() | ||
| } |
There was a problem hiding this comment.
I'd prefer to move the to_vec() call out of this function and return a fixed length byte array. This being fixed length is security relevant, so I wouldn't hide it behind a vec conversion
| @@ -24,15 +25,20 @@ pub struct WeightedSigners { | |||
|
|
|||
| impl WeightedSigners { | |||
| pub fn hash(&self) -> Hash { | |||
| let mut encoded = Vec::new(); | |||
| let mut encoded: Vec<Vec<u8>> = Vec::new(); | |||
There was a problem hiding this comment.
Do you need Vec<Vec<u8>>? digest takes a Ref<[u8]>. If possible I'd prefer to use Vec<&[u8]> as before. Defining encoded as mutable with nested vectors allows you to modify the vector elements as well I believe, which seems dangerous
There was a problem hiding this comment.
The reason I didn't use Vec<&[u8]> is because of the needed call to usize_to_u32 which is done in the for loop. That creates a new owned variable that doesn't live long enough, hence I needed to change this to a Vec<Vec<u8>>.
The solution would have been to save the length in the WeightedSigner & WeightedSigners structs so I can reference it, but I don't think that is a good idea.
Description
As per the Gateway contract audit, the signers hash computation has changed to use MultiversX nested encoding. Since I didn't want to add a new Rust dependency just for this and further complicate the code, a simple manual solution was used instead which yields the same result.