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.

Sign up for GitHub

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

Jump to bottom

GPT Review not-so-smart-contracts/cosmos #276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

GPT Review not-so-smart-contracts/cosmos #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3862ce
GPT Review
0xPhaze Apr 12, 2023
2e75f53
Run format
0xPhaze Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions not-so-smart-contracts/cosmos/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
# (Not So) Smart Cosmos

This repository contains examples of common Cosmos applications vulnerabilities, including code from real applications. Use Not So Smart Cosmos to learn about Cosmos (Tendermint) vulnerabilities, as a reference when performing security reviews, and as a benchmark for security and analysis tools.
This repository contains examples of common Cosmos application vulnerabilities, including code from real applications. Use Not So Smart Cosmos to learn about Cosmos (Tendermint) vulnerabilities, as a reference for conducting security reviews, and as a benchmark for security and analysis tools.

## Features

Each _Not So Smart Cosmos_ includes a standard set of information:
Each _Not So Smart Cosmos_ entry provides a standard set of information:

- Description of the vulnerability type
- Attack scenarios to exploit the vulnerability
- Recommendations to eliminate or mitigate the vulnerability
- Recommendations for eliminating or mitigating the vulnerability
- Real-world contracts that exhibit the flaw
- References to third-party resources with more information

## Vulnerabilities

| Not So Smart Contract | Description |
| -------------------------------------------------------- | --------------------------------------------------------------------------------------------- |
| [Incorrect signers](incorrect_getsigners) | Broken access controls due to incorrect signers validation |
| [Non-determinism](non_determinism) | Consensus failure because of non-determinism |
| [Not prioritized messages](messages_priority) | Risks arising from usage of not prioritized message types |
| [Slow ABCI methods](abci_fast) | Consensus failure because of slow ABCI methods |
| [ABCI methods panic](abci_panic) | Chain halt due to panics in ABCI methods |
| [Broken bookkeeping](broken_bookkeeping) | Exploit mismatch between different modules' views on balances |
| [Rounding errors](rounding_errors) | Bugs related to imprecision of finite precision arithmetic |
| [Unregistered message handler](unregistered_msg_handler) | Broken functionality because of unregistered msg handler |
| [Missing error handler](missing_error_handler) | Missing error handling leads to successful execution of a transaction that should have failed |
| Not So Smart Contract | Description |
| -------------------------------------------------------- | -------------------------------------------------------------------------------------------- |
| [Incorrect signers](incorrect_getsigners) | Broken access controls due to incorrect signer validation |
| [Non-determinism](non_determinism) | Consensus failure due to non-determinism |
| [Not prioritized messages](messages_priority) | Risks arising from use of non-prioritized message types |
| [Slow ABCI methods](abci_fast) | Consensus failure due to slow ABCI methods |
| [ABCI methods panic](abci_panic) | Chain halt due to panic in ABCI methods |
| [Broken bookkeeping](broken_bookkeeping) | Exploiting mismatch between different modules' views on balances |
| [Rounding errors](rounding_errors) | Bugs related to imprecision in finite precision arithmetic |
| [Unregistered message handler](unregistered_msg_handler) | Broken functionality due to unregistered msg handler |
| [Missing error handler](missing_error_handler) | Missing error handling results in the successful execution of a transaction that should fail |

## Credits

These examples are developed and maintained by [Trail of Bits](https://www.trailofbits.com/).

If you have questions, problems, or just want to learn more, then join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly.
If you have questions, encounter problems, or want to learn more, join the #ethereum channel on the [Empire Hacking Slack](https://empireslacking.herokuapp.com/) or [contact us](https://www.trailofbits.com/contact/) directly.
17 changes: 8 additions & 9 deletions not-so-smart-contracts/cosmos/abci_fast/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
# Slow ABCI methods
# Slow ABCI Methods

ABCI methods (like `EndBlocker`) [are not constrained by `gas`](https://docs.cosmos.network/v0.45/basics/app-anatomy.html#beginblocker-and-endblocker). Therefore, it is essential to ensure that they always will finish in a reasonable time. Otherwise, the chain will halt.
ABCI methods (such as `EndBlocker`) [are not constrained by `gas`](https://docs.cosmos.network/v0.45/basics/app-anatomy.html#beginblocker-and-endblocker). Therefore, it is crucial to ensure that they always finish in a reasonable time; otherwise, the chain will halt.

## Example

Below you can find part of a tokens lending application. Before a block is executed, the `BeginBlocker` method charges an interest for each borrower.
Below is a part of a token lending application. The `BeginBlocker` method charges interest for each borrower before a block is executed.

```go
func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
Expand All @@ -28,15 +28,14 @@ func accrueInterest(ctx sdk.Context, k keeper.Keeper) {
}
```

The `accrueInterest` contains multiple nested for loops and is obviously too complex to be efficient. Mischievous
users can take a lot of small loans to slow down computation to a point where the chain is not able to keep up with blocks production and halts.
The `accrueInterest` function contains multiple nested for loops, making it too complex to be efficient. Malicious users can take many small loans to slow down computation to the point where the chain cannot keep up with block production and halts.

## Mitigations

- Estimate computational complexity of all implemented ABCI methods and ensure that they will scale correctly with the application's usage growth
- Implement stress tests for the ABCI methods
- [Ensure that minimal fees are enforced on all messages](https://docs.cosmos.network/v0.46/basics/gas-fees.html#introduction-to-gas-and-fees) to prevent spam
- Estimate the computational complexity of all implemented ABCI methods and ensure that they will scale correctly with the application's usage growth.
- Implement stress tests for the ABCI methods.
- [Ensure that minimal fees are enforced on all messages](https://docs.cosmos.network/v0.46/basics/gas-fees.html#introduction-to-gas-and-fees) to prevent spam.

## External examples
## External Examples

- [Gravity Bridge's `slashing` method was executed in the `EndBlocker` and contained a computationally expensive, nested loop](https://github.com/althea-net/cosmos-gravity-bridge/issues/347).
18 changes: 9 additions & 9 deletions not-so-smart-contracts/cosmos/abci_panic/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# ABCI methods panic
# ABCI Methods Panic

A `panic` inside an ABCI method (e.g., `EndBlocker`) will stop the chain. There should be no unanticipated `panic`s in these methods.

Some less expected `panic` sources are:
Some less expected sources of `panic` are:

- [`Coins`, `DecCoins`, `Dec`, `Int`, and `UInt` types panics a lot](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/coin.go#L68), [for example on overflows](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/dec_coin.go#L105) and [rounding errors](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/decimal.go#L648)
- [`Coins`, `DecCoins`, `Dec`, `Int`, and `UInt` types panic a lot](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/coin.go#L68), [for example, on overflows](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/dec_coin.go#L105) and [rounding errors](https://github.com/cosmos/cosmos-sdk/blob/afbb0bd1941f7ad36e086913153af02eb6a68f5a/types/decimal.go#L648)
- [`new Dec` panics](https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types@v0.45.5#Dec)
- [`x/params`'s `SetParamSet` panics if arguments are invalid](https://github.com/cosmos/cosmos-sdk/blob/1b1dbf8ab722e4689e14a5a2a1fc433b69bc155e/x/params/doc.go#L107-L108)

## Example

The application below enforces limits on how much coins can be borrowed globally. If the `loan.Borrowed` array of Coins can be forced to be not-sorted (by coins' denominations), the `Add` method will `panic`.
The application below enforces limits on how many coins can be borrowed globally. If the `loan.Borrowed` array of Coins can be forced to be unsorted (by coins' denominations), the `Add` method will `panic`.

Moreover, the `Mul` may panic if some asset's price becomes large.
Moreover, the `Mul` method may panic if some asset's price becomes too large.

```go
func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
Expand All @@ -21,14 +21,14 @@ func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
}
}

func validateTotalBorrows(ctx sdk.Context, k keeper.Keeper) {
func validateTotalBorrows(ctx sdk.Context, k keeper.Keeper) bool {
total := sdk.NewCoins()
for _, loan := range k.GetUsersLoans() {
total.Add(loan.Borrowed...)
}

for _, totalOneAsset := range total {
if totalOneAsset.Amount.Mul(k.GetASsetPrice(totalOneAsset.Denom)).GTE(k.GetGlobalMaxBorrow()) {
if totalOneAsset.Amount.Mul(k.GetAssetPrice(totalOneAsset.Denom)).GTE(k.GetGlobalMaxBorrow()) {
return false
}
}
Expand All @@ -39,9 +39,9 @@ func validateTotalBorrows(ctx sdk.Context, k keeper.Keeper) {
## Mitigations

- [Use CodeQL static analysis](https://github.com/crypto-com/cosmos-sdk-codeql/blob/main/src/beginendblock-panic.ql) to detect `panic`s in ABCI methods
- Review the code against unexpected `panic`s
- Review the code for unexpected `panic`s

## External examples
## External Examples

- [Gravity Bridge can `panic` in multiple locations in the `EndBlocker` method](https://giters.com/althea-net/cosmos-gravity-bridge/issues/348)
- [Agoric `panic`s purposefully if the `PushAction` method returns an error](https://github.com/Agoric/agoric-sdk/blob/9116ede69169ebb252faf069d90022e8e05c6a4e/golang/cosmos/x/vbank/module.go#L166)
Expand Down
30 changes: 15 additions & 15 deletions not-so-smart-contracts/cosmos/broken_bookkeeping/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
# Broken Bookkeeping
# Handling Broken Bookkeeping

The `x/bank` module is the standard way to manage tokens in a cosmos-sdk based applications. The module allows to mint, burn, and transfer coins between both users' and modules' accounts. If an application implements its own, internal bookkeeping, it must carefully use the `x/bank`'s features.
The `x/bank` module is the standard way to manage tokens in applications based on the Cosmos SDK. It allows minting, burning, and transferring coins between both user accounts and module accounts. If an application uses its own internal bookkeeping, it must carefully utilize the `x/bank` features.

## Example

An application enforces the following invariant as a sanity-check: amount of tokens owned by a module equals to the amount of tokens deposited by users via the custom `x/hodl` module.
An application enforces the following invariant as a sanity check: the amount of tokens owned by a module equals the amount of tokens deposited by users via the custom `x/hodl` module.

```go
func BalanceInvariant(k Keeper) sdk.Invariant {
Expand All @@ -25,15 +25,15 @@ func BalanceInvariant(k Keeper) sdk.Invariant {
}
```

A spiteful user can simply transfer a tiny amount of BTC tokens directly to the `x/hodl` module via a message to the `x/bank` module. That would bypass accounting of the `x/hodl`, so the `GetTotalDeposited` function would report a not-updated amount, smaller than the module's `SpendableCoins`.
A malicious user can simply transfer a tiny amount of BTC tokens directly to the `x/hodl` module via a message to the `x/bank` module. This would bypass the accounting of the `x/hodl`, and the `GetTotalDeposited` function would report an outdated amount, smaller than the module's `SpendableCoins`.

Because an invariant's failure stops the chain, the bug constitutes a simple Denial-of-Service attack vector.
Since an invariant's failure stops the chain, this bug represents a simple Denial-of-Service attack vector.

## Example 2

An example application implements a lending platform. It allows users to deposit Tokens in exchange for xTokens - similarly to the [Compound's cTokens](https://compound.finance/docs/ctokens#exchange-rate). Token:xToken exchange rate is calculated as `(amount of Tokens borrowed + amount of Tokens held by the module account) / (amount of uTokens in circulation)`.
Suppose an application implements a lending platform, allowing users to deposit Tokens in exchange for xTokens - similar to [Compound's cTokens](https://compound.finance/docs/ctokens#exchange-rate). The Token:xToken exchange rate is calculated as `(amount of Tokens borrowed + amount of Tokens held by the module account) / (amount of uTokens in circulation)`.

Implementation of the `GetExchangeRate` method computing an exchange rate is presented below.
The implementation of the `GetExchangeRate` method for computing an exchange rate is presented below.

```go
func (k Keeper) GetExchangeRate(tokenDenom string) sdk.Coin {
Expand All @@ -48,22 +48,22 @@ func (k Keeper) GetExchangeRate(tokenDenom string) sdk.Coin {

```

A malicious user can screw an exchange rate in two ways:
A malicious user can manipulate the exchange rate in two ways:

- by force-sending Tokens to the module, changing the `tokensHeld` value
- by transferring uTokens to another chain via IBC, chaning `uTokensInCirculation` value
- by transferring uTokens to another chain via IBC, changing the `uTokensInCirculation` value

The first "attack" could be pulled of by sending [`MsgSend`](https://docs.cosmos.network/main/modules/bank#msgsend) message. However, it would be not profitable (probably), as executing it would irreversibly decrease an attacker's resources.
The first "attack" could be executed by sending a [`MsgSend`](https://docs.cosmos.network/main/modules/bank#msgsend) message but would likely be unprofitable, as it would irreversibly decrease the attacker's resources.

The second one works because the IBC module [burns transferred coins in the source chain](https://github.com/cosmos/ibc-go/blob/48a6ae512b4ea42c29fdf6c6f5363f50645591a2/modules/apps/transfer/keeper/relay.go#L135-L136) and mints corresponding tokens in the destination chain. Therefore, it will decrease the supply reported by the `x/bank` module, increasing the exchange rate. After the attack the malicious user can just transfer back uTokens.
The second attack works because the IBC module [burns transferred coins in the source chain](https://github.com/cosmos/ibc-go/blob/48a6ae512b4ea42c29fdf6c6f5363f50645591a2/modules/apps/transfer/keeper/relay.go#L135-L136) and mints corresponding tokens in the destination chain. As a result, the supply reported by the `x/bank` module decreases, increasing the exchange rate. The malicious user can then easily transfer back the uTokens.

## Mitigations

- Use [`Blocklist` to prevent unexpected token transfers](https://docs.cosmos.network/v0.45/modules/bank/02_keepers.html#blocklisting-addresses) to specific addresses
- Use [`SendEnabled` parameter to prevent unexpected transfers](https://docs.cosmos.network/v0.45/modules/bank/05_params.html#parameters) of specific tokens (denominations)
- Ensure that the blocklist is explicitly checked [whenever a new functionality allowing for tokens transfers is implemented](https://github.com/cosmos/cosmos-sdk/issues/8463#issuecomment-801046285)
- Use [`Blocklist`](https://docs.cosmos.network/v0.45/modules/bank/02_keepers.html#blocklisting-addresses) to prevent unexpected token transfers to specific addresses
- Use [`SendEnabled`](https://docs.cosmos.network/v0.45/modules/bank/05_params.html#parameters) parameter to prevent unexpected transfers of specific tokens (denominations)
- Ensure that the blocklist is explicitly checked [whenever a new functionality allowing for token transfers is implemented](https://github.com/cosmos/cosmos-sdk/issues/8463#issuecomment-801046285)

## External examples

- [Umee was vulnerable to the token:uToken exchange rate manipulation](https://github.com/trailofbits/publications/blob/master/reviews/Umee.pdf) (search for finding TOB-UMEE-21).
- [Umee was vulnerable to token:uToken exchange rate manipulation](https://github.com/trailofbits/publications/blob/master/reviews/Umee.pdf) (search for finding TOB-UMEE-21)
- [Desmos incorrectly blocklisted addresses](https://github.com/desmos-labs/desmos/blob/e3c89e2f9ddd5dfde5d11c3ad5319e3c249cacb3/CHANGELOG.md#version-0154) (check app.go file in [the commits diff](https://github.com/desmos-labs/desmos/compare/v0.15.3...v0.15.4))
Loading