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.

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

Problem: prevent int overflow #431

Closed

Conversation

lukitsbrian
Copy link

Description

This PR implements a fix to prevent an int overflow when converting a big.Int to sdkmath.Int.

Upon heavy load testing, there is a possibility that the feemarket eip1559 base fee implementation can crash.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/feemarket/keeper/keeper.go Outdated Show resolved Hide resolved
@lukitsbrian lukitsbrian requested a review from mmsqe March 21, 2024 01:30
@yihuang
Copy link
Collaborator

yihuang commented Mar 21, 2024

overflows 256bits integer?

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 62.53%. Comparing base (00b9e0e) to head (c0807a7).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #431      +/-   ##
===========================================
- Coverage    62.53%   62.53%   -0.01%     
===========================================
  Files          130      130              
  Lines        12495    12503       +8     
===========================================
+ Hits          7814     7819       +5     
- Misses        4135     4136       +1     
- Partials       546      548       +2     
Files Coverage Δ
x/feemarket/keeper/keeper.go 70.58% <100.00%> (+2.50%) ⬆️
x/feemarket/keeper/params.go 46.15% <40.00%> (-2.42%) ⬇️

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff --git a/types/int.go b/types/int.go
index c70d1b6a..9342b836 100644
--- a/types/int.go
+++ b/types/int.go
@@ -19,13 +19,23 @@ import (
 	fmt "fmt"
 	math "math"
 	"math/big"
+	"math/bits"
 
 	errorsmod "cosmossdk.io/errors"
 	sdkmath "cosmossdk.io/math"
 	errortypes "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
-const maxBitLen = 256
+const (
+	maxWordLen = sdkmath.MaxBitLen / bits.UintSize
+)
+
+var MaxInt256 *big.Int
+
+func init() {
+	var tmp big.Int
+	MaxInt256 = tmp.Lsh(big.NewInt(1), sdkmath.MaxBitLen).Sub(&tmp, big.NewInt(1))
+}
 
 // SafeInt64 checks for overflows while casting a uint64 to int64 value.
 func SafeInt64(value uint64) (int64, error) {
@@ -52,7 +62,28 @@ func SafeNewIntFromBigInt(i *big.Int) (sdkmath.Int, error) {
 	return sdkmath.NewIntFromBigInt(i), nil
 }
 
+// SaturatedNewInt constructs Int from big.Int, truncate if more than 256bits
+func SaturatedNewInt(i *big.Int) sdkmath.Int {
+	if !IsValidInt256(i) {
+		i = MaxInt256
+	}
+	return sdkmath.NewIntFromBigInt(i)
+}
+
 // IsValidInt256 check the bound of 256 bit number
 func IsValidInt256(i *big.Int) bool {
-	return i == nil || i.BitLen() <= maxBitLen
+	return i == nil || !bigIntOverflows(i)
+}
+
+// check if the big int overflows,
+// NOTE: copied from cosmos-sdk.
+func bigIntOverflows(i *big.Int) bool {
+	// overflow is defined as i.BitLen() > MaxBitLen
+	// however this check can be expensive when doing many operations.
+	// So we first check if the word length is greater than maxWordLen.
+	// However the most significant word could be zero, hence we still do the bitlen check.
+	if len(i.Bits()) > maxWordLen {
+		return i.BitLen() > sdkmath.MaxBitLen
+	}
+	return false
 }
diff --git a/types/int_test.go b/types/int_test.go
new file mode 100644
index 00000000..b751cacc
--- /dev/null
+++ b/types/int_test.go
@@ -0,0 +1,18 @@
+package types
+
+import (
+	"math/big"
+	"testing"
+
+	sdkmath "cosmossdk.io/math"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMaxInt256(t *testing.T) {
+	maxInt256Plus1 := new(big.Int).Add(MaxInt256, big.NewInt(1))
+	require.Equal(t, sdkmath.MaxBitLen, MaxInt256.BitLen())
+	require.Equal(t, sdkmath.MaxBitLen+1, maxInt256Plus1.BitLen())
+
+	require.True(t, IsValidInt256(MaxInt256))
+	require.False(t, IsValidInt256(maxInt256Plus1))
+}
diff --git a/x/feemarket/keeper/params.go b/x/feemarket/keeper/params.go
index eab87a39..9a04a04e 100644
--- a/x/feemarket/keeper/params.go
+++ b/x/feemarket/keeper/params.go
@@ -18,7 +18,7 @@ package keeper
 import (
 	"math/big"
 
-	sdkmath "cosmossdk.io/math"
+	ethermint "github.com/evmos/ethermint/types"
 	"github.com/evmos/ethermint/x/feemarket/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -71,7 +71,7 @@ func (k Keeper) GetBaseFee(ctx sdk.Context) *big.Int {
 // SetBaseFee set's the base fee in the store
 func (k Keeper) SetBaseFee(ctx sdk.Context, baseFee *big.Int) {
 	params := k.GetParams(ctx)
-	params.BaseFee = sdkmath.NewIntFromBigInt(baseFee)
+	params.BaseFee = ethermint.SaturatedNewInt(baseFee)
 	err := k.SetParams(ctx, params)
 	if err != nil {
 		return

I did a cleaner version of it, can you use this version if you likes it.

@yihuang
Copy link
Collaborator

yihuang commented Mar 25, 2024

fixed in #433

@yihuang yihuang closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants