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

QA Report #160

Open
code423n4 opened this issue Jul 2, 2022 · 1 comment
Open

QA Report #160

code423n4 opened this issue Jul 2, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Low/Non-critical Vulnerabilities

Low

Use of deprecated functions

The ioutil package is deprecated. os.ReadFile should be used instead of ioutil.ReadFile

Reference:

Deprecation of ioutil library

The following lines of code are affected:

x/unigov/client/cli/utils.go:17:    contents, err := ioutil.ReadFile(filepath.Clean(metadataFile))
x/unigov/client/cli/utils.go:38:    contents, err := ioutil.ReadFile(filepath.Clean(metadataFile))
x/vesting/client/cli/tx.go:66:  contents, err := ioutil.ReadFile(filepath.Clean(path))
x/erc20/client/cli/utils.go:15: contents, err := ioutil.ReadFile(filepath.Clean(metadataFile))

Open TODOs

Comments in production code should not contain developer discussion or notes about known bugs or problems. These issues should be tracked elsewhere and resolved before being deployed.

Comments of this kind can also indicate potential avenues of attack for an adversary.

The following lines are affected:

app/app.go:813:		// TODO: Record the count along with the code and or reason so as to display
app/ante/comission.go:14:// TODO: remove once Cosmos SDK is upgraded to v0.46
app/ante/vesting.go:78:// TODO: remove once Cosmos SDK is upgraded to v0.46
app/tps_counter.go:113:		// TODO: Perhaps log this?
testutil/network/network.go:71:	LegacyAmino       *codec.LegacyAmino // TODO: Remove!
x/vesting/types/schedule.go:167:// TODO: rename and add comprehensive comments, this is currently not maintainable
cmd/cantod/root.go:94:			// TODO: define our own token
cmd/cantod/root.go:101:	// TODO: double-check
cmd/cantod/main.go:35:	// TODO fix
cmd/config/observability.go:28:	// // TODO: Derive the Prometheus observability exporter from the Evmos config.

Use of dependencies with known vulnerabilities

Packages used by the project are known to contain security vulnerabilities. It is recommended to update these dependencies in order to avoid issues related to these vulnerabilities.

In addition, it is advised to put in place an automated process to flag vulnerable components and fix them during the build process.

The following vulnerable packages are installed:

  • tendermint@v0.34.19
  • go-ethereum@v1.10.16

For more information consult the following resources:

Tendermint GitHub Issue
Go Ethereum security advisory

Open TODOs

Comments in production code should not contain developer discussion or notes about known bugs or problems. These issues should be tracked elsewhere and resolved before being deployed.

Comments of this kind can also indicate potential avenues of attack for an adversary.

The following lines are affected:

testutil/network/network.go:    LegacyAmino       *codec.LegacyAmino // TODO: Remove!
x/vesting/types/schedule.go:// TODO: rename and add comprehensive comments, this is currently not maintainable
app/ante/comission.go:// TODO: remove once Cosmos SDK is upgraded to v0.46
app/ante/vesting.go:// TODO: remove once Cosmos SDK is upgraded to v0.46
app/app.go:             // TODO: Record the count along with the code and or reason so as to display
app/tps_counter.go:             // TODO: Perhaps log this?
cmd/config/observability.go:    // // TODO: Derive the Prometheus observability exporter from the Evmos config.
cmd/cantod/root.go:                     // TODO: define our own token
cmd/cantod/root.go:     // TODO: double-check
cmd/cantod/main.go:     // TODO fix

Non-critical

Panic used as error handling

Avoid using panic in production code. Calls to panic can reveal sensitive information about the system via the output of stack traces. When errors are not handled in a recoverable way it is possible for the software to reach undefined behaviour or even crash.

Instead of using panic, incorporate custom errors according to the best practices of the CosmosSDK.

More information about custom errors can be found at the following resource:

CosmosSDK: Errors

The use of panic was identified at the following lines in the codebase:

x/unigov/genesis.go:17:		panic("the UniGov module account has not been set")
x/unigov/client/cli/tx.go:118:		panic(err)
x/unigov/client/cli/tx.go:121:		panic(err)
x/unigov/client/cli/tx.go:124:		panic(err)
x/unigov/client/cli/tx.go:204:		panic(err)
x/unigov/client/cli/tx.go:207:		panic(err)
x/unigov/client/cli/tx.go:210:		panic(err)
x/unigov/simulation/simap.go:12:		panic(err)
x/recovery/module.go:75:		panic(err)
x/inflation/module.go:79:		panic(err)
x/recovery/keeper/keeper.go:57:		panic("ICS4 wrapper already set")
x/epochs/module.go:79:		panic(err)
x/vesting/module.go:66:		panic(err)
x/epochs/keeper/keeper.go:31:		panic("cannot set epochs hooks twice")
x/vesting/keeper/msg_server.go:228:		panic("bad start time calculation")
x/inflation/genesis.go:19:		panic("the inflation module account has not been set")
x/inflation/keeper/epoch_mint_provisions.go:21:		panic(fmt.Errorf("unable to unmarshal epochMintProvision value: %w", err))
x/inflation/keeper/epoch_mint_provisions.go:31:		panic(fmt.Errorf("unable to marshal amount value: %w", err))
x/inflation/keeper/keeper.go:39:		panic("the mint module account has not been set")
x/incentives/module.go:75:		panic(err)
x/inflation/keeper/hooks.go:42:		panic("the epochMintProvision was not found")
x/inflation/keeper/hooks.go:47:		panic(err)
x/incentives/genesis.go:22:		panic("the incentives module account has not been set")
x/incentives/client/cli/tx.go:91:		panic(err)
x/incentives/client/cli/tx.go:94:		panic(err)
x/incentives/client/cli/tx.go:97:		panic(err)
x/incentives/client/cli/tx.go:163:		panic(err)
x/incentives/client/cli/tx.go:166:		panic(err)
x/incentives/client/cli/tx.go:169:		panic(err)
x/incentives/keeper/allocation_meters.go:51:		panic(fmt.Errorf("unable to unmarshal amount value %v", err))
x/incentives/keeper/allocation_meters.go:63:		panic(fmt.Errorf("unable to marshal amount value %v", err))
x/incentives/keeper/epoch_hooks.go:27:		panic(err)
x/erc20/genesis.go:23:		panic("the erc20 module account has not been set")
x/fees/module.go:80:		panic(err)
x/erc20/module.go:74:		panic(err)
x/erc20/module.go:139:		panic(fmt.Errorf("failed to migrate %s to v2: %w", types.ModuleName, err))
x/erc20/client/cli/tx.go:222:		panic(err)
x/erc20/client/cli/tx.go:225:		panic(err)
x/erc20/client/cli/tx.go:228:		panic(err)
x/erc20/client/cli/tx.go:288:		panic(err)
x/erc20/client/cli/tx.go:291:		panic(err)
x/erc20/client/cli/tx.go:294:		panic(err)
x/erc20/client/cli/tx.go:354:		panic(err)
x/erc20/client/cli/tx.go:357:		panic(err)
x/erc20/client/cli/tx.go:360:		panic(err)
app/tps_counter.go:77:				panic(err)
app/tps_counter.go:83:				panic(err)
contracts/erc20burnable.go:21:		panic(err)
app/forks.go:30:			panic(err)
app/forks.go:50:			panic(err)
contracts/erc20DirectBalanceManipulation.go:31:		panic(err)
contracts/erc20DirectBalanceManipulation.go:35:		panic("load contract failed")
contracts/erc20.go:29:		panic(err)
contracts/erc20.go:33:		panic("load contract failed")
contracts/erc20maliciousdelayed.go:31:		panic(err)
contracts/erc20maliciousdelayed.go:35:		panic("load contract failed")
app/test_helpers.go:70:				panic(err)
app/test_helpers.go:77:			panic(err)
contracts/ProposalStore.go:21:		panic(err)
ibc/testing/chain.go:55:		panic(err)
app/app.go:148:		panic(err)
app/app.go:769:		panic(err)
app/app.go:829:		panic(err)
app/app.go:981:		panic(err)
app/app.go:1050:		panic(fmt.Errorf("failed to read upgrade info from disk: %w", err))
cmd/config/observability.go:33:	// 		panic(err)
cmd/config/config.go:51:		panic(err)
cmd/config/config.go:55:		panic(err)
cmd/cantod/main.go:37:	// 	panic(err)
cmd/cantod/root.go:134:		panic(err)
cmd/cantod/root.go:204:		panic(fmt.Errorf("unknown app config type %T", customAppConfig))
cmd/cantod/root.go:232:		panic(err)
cmd/cantod/root.go:238:		panic(err)
cmd/cantod/root.go:242:		panic(err)
@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jul 2, 2022
code423n4 added a commit that referenced this issue Jul 2, 2022
@GalloDaSballo
Copy link
Collaborator

Use of deprecated functions

Valid Low

Open TODOs

NC

Use of dependencies with known vulnerabilities

In lack of POC I cannot count this one as valid, please show how the vulnerability is exposed in the code in scope next time

Panic used as error handling

Valid Refactoring

1 L 1R 1NC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

2 participants