Onboarding::Types::Params lacks multiple validations

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/types/params.go#L89

Vulnerability details

Impact

• Validate() is missing the call to validateWhitelistedChannels, which means anything could be injected in the whitelist channel which seems concerning.
• validateAutoSwapThreshold() is allowing 0, which doesn't make sense and would effectivelly disable the swaping in onboarding as if standardCoinBalance.LT(autoSwapThreshold) would always be false.
• validateWhitelistedChannels() is missing multiple verifications as it allows the following:
  • empty list
  • list with empty entry
  • list with duplicates

While the impact on those seems Low/Medium, I'm reporting this as Medium as a whole as I feel it is more tilting toward it.

Proof of Concept

Please see recommended section.

Tools Used

Go v1.20 and Goland IDE

Recommended Mitigation Steps

I would recommend to add the following code and test coverage which resolve the issues I'm reporting and also verify them.

CODE DIFF

diff --git a/Canto/x/onboarding/types/params.go b/Canto/x/onboarding/types/params.go
index a146e34..918bf50 100644
--- a/Canto/x/onboarding/types/params.go
+++ b/Canto/x/onboarding/types/params.go
@@ -55,11 +55,27 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
 }

 func validateWhitelistedChannels(i interface{}) error {
-       _, ok := i.([]string)
+       v, ok := i.([]string)
        if !ok {
                return fmt.Errorf("invalid parameter type: %T", i)
        }

+       if len(v) == 0 {
+               return fmt.Errorf("whitelist cannot be empty")
+       }
+
+       // check for any empty entries, which should not be allowed. Also detect any duplicate entry
+       uniqueChannel := map[string]bool{}
+       for _, channel := range v {
+               if len(channel) == 0 {
+                       return fmt.Errorf("channel cannot be empty")
+               }
+               if _, exist := uniqueChannel[channel]; exist {
+                       return fmt.Errorf("channel duplicated : %s", channel)
+               }
+               uniqueChannel[channel] = true
+       }
+
        return nil
 }

@@ -78,8 +94,8 @@ func validateAutoSwapThreshold(i interface{}) error {
                return fmt.Errorf("invalid parameter type: %T", i)
        }

-       if v.IsNegative() {
-               return fmt.Errorf("auto swap threshold must be positive: %s", v.String())
+       if v.IsZero() || v.IsNegative() {
+               return fmt.Errorf("auto swap threshold must be greater than 0 : %s", v.String())
        }

        return nil
@@ -93,5 +109,8 @@ func (p Params) Validate() error {
        if err := validateAutoSwapThreshold(p.AutoSwapThreshold); err != nil {
                return err
        }
+       if err := validateWhitelistedChannels(p.WhitelistedChannels); err != nil {
+               return err
+       }
        return nil
 }

UNIT TEST DIFF

diff --git a/Canto/x/onboarding/types/params_test.go b/Canto/x/onboarding/types/params_test.go
index 496c7b2..2bc67a8 100644
--- a/Canto/x/onboarding/types/params_test.go
+++ b/Canto/x/onboarding/types/params_test.go
@@ -31,6 +31,31 @@ func TestParamsValidate(t *testing.T) {
                        NewParams(true, sdk.NewInt(10000), []string{"channel-0"}),
                        false,
                },
+               {
+                       "custom params v2",
+                       NewParams(false, sdk.NewIntWithDecimal(2, 18), []string{"channel-1"}),
+                       false,
+               },
+               {
+                       "bad params for AutoSwapThreshold",
+                       NewParams(false, sdk.NewInt(0), []string{"channel-1"}),
+                       true,
+               },
+               {
+                       "bad params for WhitelistedChannels (empty)",
+                       NewParams(false, sdk.NewIntWithDecimal(1, 18), []string{}),
+                       true,
+               },
+               {
+                       "bad params for WhitelistedChannels (empty entry)",
+                       NewParams(false, sdk.NewIntWithDecimal(1, 18), []string{"channel-1", ""}),
+                       true,
+               },
+               {
+                       "bad params for WhitelistedChannels (duplicated entry)",
+                       NewParams(true, sdk.NewIntWithDecimal(1, 18), []string{"channel-0", "channel-1", "channel-0"}),
+                       true,
+               },
        }

        for _, tc := range testCases {

Assessed type

Invalid Validation

[c4-pre-sort]

JeffCX marked the issue as primary issue

[JeffCX]

The report points out a few missed input validation, while the impact is not clearly described, the thoughtful recommendation worth sponsor's review, the report itself is very QA though

[tkkwon1998]

All of these parameters such as white listed channels and auto swap threshold are values that can only be set through governance. As such it's really unlikely that the parameters will be invalid, since all param change proposals go through rigorous testing. I agree that this is more of a QA issue.

[c4-sponsor]

tkkwon1998 marked the issue as sponsor disputed

[c4-sponsor]

tkkwon1998 marked the issue as disagree with severity

[tkkwon1998]

meant to put disagree with severity, not sponsor disputed.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a