fix: when decoding addresses restrict to 20 bytes #56
Conversation
Signed-off-by: SamMayWork <sam.may@kaleido.io>
SamMayWork
requested review from
peterbroadhurst,
nguyer and
awrichar
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 38 38
Lines 3317 3323 +6
=======================================
+ Hits 3314 3320 +6
Misses 2 2
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
Approach looks good to me |
|
For sake of consistency, I've checked the behaviour when someone tries to force an encode operation with an address value in bytes that would exceed the 20 bytes buffer and an error is thrown if the value doesn't fit into a uint160 type as expected. |
There was a problem hiding this comment.
This looks good and makes sense to me. It is consistent with how we decode other types. I think it would be good to have @peterbroadhurst take a quick look at this also to make sure I'm not missing something here as this is pretty important, low level code.
|
Is there a chance to build a docker image from this branch? |
I think we can get this merged today which will automatically trigger a snapshot image build |
There was a problem hiding this comment.
Per the above, might need a bit more insight into the justification for the change you've made here @SamMayWork (vs. fixing decodeABIUnsignedInt)
pkg/abi/typecomponents.go
Outdated
| @@ -227,7 +227,7 @@ var ( | |||
| }, | |||
| jsonEncodingType: JSONEncodingTypeBytes, | |||
| encodeABIData: encodeABIUnsignedInteger, | |||
| decodeABIData: decodeABIUnsignedInt, | |||
| decodeABIData: decodeABIAddress, | |||
There was a problem hiding this comment.
Sorry @SamMayWork - I think I need to understand this better.
Maybe I need to work through this from base principals.
decodeABIUnsignedInt should be processing exactly 20 bytes.
The ABI specification states that address is processed identically to uint256.
So on the face of it, your change seems to sidestep a bug in decodeABIUnsignedInt by introducing a new function... but I don't understand why.
There was a problem hiding this comment.
Sorry, skipping working here before writing comment.
address should be processed identically to uint160
So I think the point here is that uintXXX processing is not correctly trimming before processing.
There was a problem hiding this comment.
This is a fair point - address is just a unit256 under the hood, so this fixed the problem seen on the BSC chain here, but if someone padded a normal unit256 with non-zero bytes, we'd actually hit the same issue.
Taking the fixed code and moving it into the baseline unit256 function...
There was a problem hiding this comment.
This issue is causing golang to panic
Then just as important. A uint256 should not panic the VM, including ffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025 (which to my knowledge is a perfectly valid uint256)
There was a problem hiding this comment.
So I'm expecting to see @SamMayWork :
- Processing of
ffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025as auint160resulting in0xab0974bbed8afc5212e951c8498873319d02d025as a number - Processing of
ffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025as auint8resulting in0x25as a number - Processing of
ffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025as auint256processing0xffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025as a number - Processing of
ffffffffffffffffffffffffab0974bbed8afc5212e951c8498873319d02d025as anaddressprocessing0xab0974bbed8afc5212e951c8498873319d02d025with the semantic thatuint160andaddressare equivalent at this layer of code
There was a problem hiding this comment.
Should be resolved by 711fd0f
Signed-off-by: SamMayWork <sam.may@kaleido.io>
| cv.Value = new(big.Int).SetBytes(block[headPosition : headPosition+32]) | ||
|
|
||
| // When we're reading bytes, need to make sure we're reading the correct size number of bytes for the uint size | ||
| cv.Value = new(big.Int).SetBytes(block[headPosition+(32-(int(component.m/8))) : headPosition+32]) |
|
@wpater - this docker tag should have the fix now: |
|
It's up and running for over 30mins - looks fine from my perspective |
ref: https://discordapp.com/channels/905194001349627914/928377875827154984/1194272891957694614
When indexing the BSC chain, it's been reported that the EVM connector crashes when indexing this transaction. This is because the padding scheme used in the transaction is not using zero-bytes to pad out the value. When we parse the entire 32 bytes from the chunk into an integer, the padding non-zero bytes are contributing to the integer value, causing the parsed address value to overflow what can be actually been shown in 20 bytes. This issue is causing golang to panic and the EVM connector to crash.
The proposed fix is to decode addresses only on the right-most 20 bytes, and ignore the front 12 bytes from the chunk.