code423n4 commented 2 years ago

Handle

defsec

Vulnerability details

Impact

During the manual code review, It has been observed that on the cosmos side Coin amount has not been checked on the token definition. That can use misfunctionality on the bridge. Although zero amount definition fee will be calculated. That can cause lose of user funds.

Proof of Concept

Navigate to "https://github.com/althea-net/cosmos-gravity-bridge/blob/main/module/x/gravity/types/ethereum.go" Line #69.
On the following code, ValidateBasic function does not validate amount.

// ValidateBasic permforms stateless validation
func (e *ERC20Token) ValidateBasic() error {
    if err := ValidateEthAddress(e.Contract); err != nil {
        return sdkerrors.Wrap(err, "ethereum address")
    }
    // TODO: Validate all the things
    return nil
}

Tools Used

Recommended Mitigation Steps

Add the following validation steps on the ValidationBasic function.

    if !m.Amount.IsValid() {
        return cosmos.ErrInvalidCoins("coins must be valid")
    }

    if !m.Amount.IsAllPositive() {
        return cosmos.ErrInvalidCoins("coins must be positive")
    }

jkilpatr commented 2 years ago

+1 no disagreement here, nice catch of an oversight.

albertchon commented 2 years ago

@jkilpatr is this actually used anywhere? I don't see it really used in the core code besides the tests.

and if so it deserves a N classification imo

jkilpatr commented 2 years ago

Double checking the actual commit hash for the audit it doesn't seem to be used.

So I suppose you are right, but we have used this bug report to improve that check and now we are using it everywhere we can. I would say this report was useful even if it's not quite a bug.

loudoguno commented 2 years ago

reopening as per judges assessment as "primary issue" on findings sheet