Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add iavl test proving forgery #583

Closed
wants to merge 3 commits into from
Closed

Add iavl test proving forgery #583

wants to merge 3 commits into from

Conversation

grepsuzette
Copy link
Contributor

This is a test of the iavl proof forgery as exploited during BSC 2022-10-07 hack.

proof_forgery_test.go comes from cosmos/iavl#582
Because of the discrepancy with gno/pkgs/iavl, it was adapted to compile.

Test now compiles, and fails the test as expected (-> vulnerable).

Output of go test pkgs/iavl/proof_forgery_test.go follows:

--- FAIL: TestProofForgery (0.00s)
    proof_forgery_test.go:69:
                Error Trace:    /home/bob/opt/src/COINS/Cosmos/GNO/gno/pkgs/iavl/proof_forgery_test.go:69
                Error:          Should be empty, but was [73 209 82 89 222 179 131 99 170 27 180 58 80 20 211 (...) 94 7 254 45 183 20 244]
                Test:           TestProofForgery
                Messages:       roothash must be empty if both left and right are set
FAIL
FAIL    command-line-arguments  0.003s
FAIL

A fix is coming.

@grepsuzette
import original iavl proof_forgery_test.go 2022-10-08
8cf8f61 
Won't compile yet.
Is a test of the iavl proof forgery as exploited during BSC 2022-10-07 hack.

Notes:
1. proof_forgery_test.go comes from cosmos/iavl#582
2. gist showing the same vuln at https://gist.github.com/samczsun/8635f49fac0ec66a5a61080835cae3db

The test is not going to compile as is, it needs some work.
@grepsuzette
proof_forgery_test.go compiles, and fails the test as expected (-> vu…
b5d72c7 
…lnerable)

output follows

--- FAIL: TestProofForgery (0.00s)
    proof_forgery_test.go:69:
                Error Trace:    /home/bob/opt/src/COINS/Cosmos/GNO/gno/pkgs/iavl/proof_forgery_test.go:69
                Error:          Should be empty, but was [73 209 82 89 222 179 131 99 170 27 180 58 80 20 211 (...) 94 7 254 45 183 20 244]
                Test:           TestProofForgery
                Messages:       roothash must be empty if both left and right are set
FAIL
FAIL    command-line-arguments  0.003s
FAIL
@grepsuzette grepsuzette requested a review from a team as a code owner March 10, 2023 04:14
grepsuzette added a commit to grepsuzette/gno that referenced this pull request Mar 10, 2023
@grepsuzette
fix vuln demonstrated by gnolang#583
508cd88 
Original fix at cosmos/iavl#582, is simply:

```
if len(pin.Left) > 0 && len(pin.Right) > 0 {
	return nil, errors.New("both left and right child hashes are set")
}
```
Our iavl functions however don't return errors. Proposing to use
`panic()` instead, as it does in other parts of this file.

More about this 2022-10-07 vuln:

https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e
https://twitter.com/buchmanster/status/1578879225574350848
@grepsuzette
Copy link
Contributor Author

After applying #584 test fails, this time with:

--- FAIL: TestProofForgery (0.00s)                                     
panic: both left and right child hashes are set [recovered]            
        panic: both left and right child hashes are set

@grepsuzette
Copy link
Contributor Author

grepsuzette commented Mar 10, 2023 •
edited

Would like to use assert.PanicsWithValue() from https://github.com/stretchr/testify/releases for the test to be clean. But seems we have an older version, only assert.Panics() is available.
Can we update stretchr/testify?
Edit: can use require.Panics() or assert.Panics() in the meantime, it's alright. Will add this to a list of separate things to ask later.

Edit: it's not ready for review yet. Test needs reorganization.

@grepsuzette
Test now passes whenever gnolang#583 is applied
07f364a 
and fails otherwise (fails when vuln not fixed)

It should now be ok for review.
@grepsuzette
Copy link
Contributor Author

It should be ready for review.

@grepsuzette grepsuzette mentioned this pull request Mar 10, 2023
Open
grepsuzette added a commit to grepsuzette/gno that referenced this pull request Mar 30, 2023
@grepsuzette
Merge the fix (gnolang#584) with the tests (gnolang#583) to have a si…
817171d 
…ngle PR
@grepsuzette grepsuzette mentioned this pull request Mar 30, 2023
Merged
@grepsuzette
Copy link
Contributor Author

I think it's more convenient to have the test in #584, for CI sake.
So closing here.

grepsuzette added a commit to grepsuzette/gno that referenced this pull request Apr 10, 2023
@grepsuzette
fix vuln demonstrated by gnolang#583
d7bcd68 
Original fix at cosmos/iavl#582, is simply:

```
if len(pin.Left) > 0 && len(pin.Right) > 0 {
	return nil, errors.New("both left and right child hashes are set")
}
```
Our iavl functions however don't return errors. Proposing to use
`panic()` instead, as it does in other parts of this file.

More about this 2022-10-07 vuln:

https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e
https://twitter.com/buchmanster/status/1578879225574350848
grepsuzette added a commit to grepsuzette/gno that referenced this pull request Apr 10, 2023
@grepsuzette
Test now passes whenever gnolang#583 is applied
81cac3b 
and fails otherwise (fails when vuln not fixed)

It should now be ok for review.
moul added a commit that referenced this pull request Sep 22, 2023
@grepsuzette @ajnavarro @moul
fix: fix vuln demonstrated by #583 (#584)
b449e89 
This is a fix for the 2022-10-07 Binance vuln demonstrated in #583.

**Original fix** was simply (cosmos/iavl#582):

```
if len(pin.Left) > 0 && len(pin.Right) > 0 {
	return nil, errors.New("both left and right child hashes are set")
}
```
Our iavl functions however don't return errors. Proposing to use
`panic()` instead, as it does in other parts of this file.

---
More about the vuln, for comments and archival:

* https://twitter.com/buchmanster/status/1578879225574350848
*
https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e

---------

Co-authored-by: grepsuzette <grepsuzette@users.noreply.github.com>
Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
gfanton pushed a commit to gfanton/gno that referenced this pull request Nov 9, 2023
@grepsuzette @ajnavarro
@moul @gfanton
fix: fix vuln demonstrated by gnolang#583 (gnolang#584)
6e447c5 
This is a fix for the 2022-10-07 Binance vuln demonstrated in gnolang#583.

**Original fix** was simply (cosmos/iavl#582):

```
if len(pin.Left) > 0 && len(pin.Right) > 0 {
	return nil, errors.New("both left and right child hashes are set")
}
```
Our iavl functions however don't return errors. Proposing to use
`panic()` instead, as it does in other parts of this file.

---
More about the vuln, for comments and archival:

* https://twitter.com/buchmanster/status/1578879225574350848
*
https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e

---------

Co-authored-by: grepsuzette <grepsuzette@users.noreply.github.com>
Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@grepsuzette grepsuzette

Labels
None yet
Projects
🚀 The Launch
Status: No status
🧐 Review Meeting
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@grepsuzette