types: ParseCoinNormalized may panic for very large values

[elias-orijtech]

Adding this testcase found by OSS-Fuzz,

diff --git types/coin_test.go types/coin_test.go
index 8252b0f1b..787c7c85d 100644
--- types/coin_test.go
+++ types/coin_test.go
@@ -777,6 +777,7 @@ func (s *coinTestSuite) TestParseCoins() {
                {"10atom10", true, sdk.Coins{{"atom10", sdk.NewInt(10)}}},
                {"200transfer/channelToA/uatom", true, sdk.Coins{{"transfer/channelToA/uatom", sdk.NewInt(200)}}},
                {"50ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", true, sdk.Coins{{"ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", sdk.NewInt(50)}}},
+               {"120000000000000000000000000000000000000000000000000000000000000000000000000000btc", false, nil},
        }

        for tcIndex, tc := range cases {

results in a panic:

$ go test -trimpath
starting test
--- FAIL: TestCoinTestSuite (0.01s)
    --- FAIL: TestCoinTestSuite/TestParseCoins (0.00s)
        suite.go:63: test panicked: NewIntFromBigInt() out of bound
            goroutine 324 [running]:
            runtime/debug.Stack()
            	runtime/debug/stack.go:24 +0x65
            github.com/stretchr/testify/suite.failOnPanic(0xc0004d8680)
            	github.com/stretchr/[email protected]/suite/suite.go:63 +0x3e
            panic({0xf4d280, 0x1384a60})
            	runtime/panic.go:838 +0x207
            github.com/cosmos/cosmos-sdk/types.NewIntFromBigInt(...)
            	github.com/cosmos/cosmos-sdk/types/int.go:112
            github.com/cosmos/cosmos-sdk/types.Dec.TruncateInt({0xbe86c5?})
            	github.com/cosmos/cosmos-sdk/types/decimal.go:686 +0x7d
            github.com/cosmos/cosmos-sdk/types.DecCoin.TruncateDecimal({{0x111be34?, 0xbe1827?}, {0xc0002974a0?}})
            	github.com/cosmos/cosmos-sdk/types/dec_coin.go:113 +0x39
            github.com/cosmos/cosmos-sdk/types.NormalizeCoins({0xc000564ba0?, 0x1, 0x2?})
            	github.com/cosmos/cosmos-sdk/types/denom.go:142 +0xe5
            github.com/cosmos/cosmos-sdk/types.ParseCoinsNormalized({0x111bde6?, 0x109d540?})
            	github.com/cosmos/cosmos-sdk/types/coin.go:840 +0x45
            github.com/cosmos/cosmos-sdk/types_test.(*coinTestSuite).TestParseCoins(0xc000142370)
            	github.com/cosmos/cosmos-sdk/types/coin_test.go:784 +0x8ae
            reflect.Value.call({0xc00032a8a0?, 0xc000732400?, 0x13?}, {0x10d470f, 0x4}, {0xc000095e70, 0x1, 0x1?})
            	reflect/value.go:556 +0x845
            reflect.Value.Call({0xc00032a8a0?, 0xc000732400?, 0xc000142370?}, {0xc0004ce670, 0x1, 0x1})
            	reflect/value.go:339 +0xbf
            github.com/stretchr/testify/suite.Run.func1(0xc0004d8680)
            	github.com/stretchr/[email protected]/suite/suite.go:158 +0x4b6
            testing.tRunner(0xc0004d8680, 0xc00056b170)
            	testing/testing.go:1439 +0x102
            created by testing.(*T).Run
            	testing/testing.go:1486 +0x35f
FAIL
exit status 1

The issue is that the maxDecBitLen limit assumes that chopping the decimal part of a maxDecBitLen-long types.Dec number will always result in a 60 (DecimalPrecisionBits) bits shorter integer, which fits in 256 bits (maxBitLen). However, because 10^18 (Precision) is less than 2^60, this is not true.

An obvious fix is to limit maxDecBitLen:

diff --git types/decimal.go types/decimal.go
index f7d7ad9ff..1cbea27a1 100644
--- types/decimal.go
+++ types/decimal.go
@@ -26,7 +26,12 @@ const (
        // Ceiling[Log2[999 999 999 999 999 999]]
        DecimalPrecisionBits = 60

-       maxDecBitLen = maxBitLen + DecimalPrecisionBits
+       // decimalTruncateBits is the minimum number of bits removed
+       // by a truncate operation. It is equal to
+       // Floor[Log2[10^Precision]].
+       decimalTruncateBits = DecimalPrecisionBits - 1
+
+       maxDecBitLen = maxBitLen + decimalTruncateBits

        // max number of iterations in ApproxRoot function
        maxApproxRootIterations = 100

However, that results in test failures because some large values no longer fit into a types.Dec:

$ go test
--- FAIL: TestDecimalTestSuite (0.00s)
    --- FAIL: TestDecimalTestSuite/TestDecEncoding (0.00s)
        decimal_test.go:544:
            	Error Trace:	decimal_test.go:544
            	Error:      	Received unexpected error:
            	            	decimal out of range; got: 316, max: 315
            	Test:       	TestDecimalTestSuite/TestDecEncoding
    --- FAIL: TestDecimalTestSuite/TestNewDecFromStr (0.00s)
        decimal_test.go:100:
            	Error Trace:	decimal_test.go:100
            	Error:      	Expected nil, but got: &errors.errorString{s:"decimal out of range; bitLen: got 316, max 315"}
            	Test:       	TestDecimalTestSuite/TestNewDecFromStr
            	Messages:   	unexpected error, decimalStr 88888888888888888888888888888888888888888888888888888888888888888888844444440, tc 17
starting test
FAIL
exit status 1
FAIL	github.com/cosmos/cosmos-sdk/types	0.372s

Version

git revision b518c84

CC @odeke-em.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

@elias-orijtech since we have a diff posted, would you be open to opening a PR?

cc @ValarDragon for decimal types visibility.

[elias-orijtech]

@elias-orijtech since we have a diff posted, would you be open to opening a PR?

Sure, but I'm waiting for someone with more knowledge before posting a PR that deletes or updates the two test cases my suggested fix break.

cc @ValarDragon for decimal types visibility.

[ValarDragon]

I'm fine w/ changing the behavior to force parsing of ints to be one minus the perceived logical maximum bit length!

Doesn't break anything Osmosis depends upon, I'd be a bit surprised if it broke things for other folks.

[alexanderbez]

@elias-orijtech I think the change is OK.