[WIP] Not so smart cosmos (#117)

Add not so smart cosmos

Author: GrosQuildu

not-so-smart-contracts/cosmos/README.md

@@ -0,0 +1,32 @@
+# (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.
+
+## Features
+
+Each _Not So Smart Cosmos_ includes a standard set of information:
+
+* Description of the vulnerability type
+* Attack scenarios to exploit the vulnerability
+* Recommendations to eliminate or mitigate 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 |
+
+## 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.

not-so-smart-contracts/cosmos/abci_fast/README.md

@@ -0,0 +1,41 @@
+# 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.
+
+## 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.
+
+```go
+func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
+    updatePrices(ctx, k)
+    accrueInterest(ctx, k)
+}
+
+func accrueInterest(ctx sdk.Context, k keeper.Keeper) {
+    for _, pool := range k.GetLendingPools() {
+        poolAssets := k.GetPoolAssets(ctx, pool.Id)
+        for userId, _ := range k.GetAllUsers() {
+            for _, asset := range poolAssets {
+                for _, loan := range k.GetUserLoans(ctx, pool, asset, userId) {
+                    if err := k.AccrueInterest(ctx, loan); err != nil {
+                        k.PunishUser(ctx, userId)
+                    }
+                }
+            }
+        }
+    }
+}
+```
+
+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.
+
+## 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
+
+## 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).

not-so-smart-contracts/cosmos/abci_panic/README.md

@@ -0,0 +1,46 @@
+# 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:
+* [`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)
+* [`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`.
+
+Moreover, the `Mul` may panic if some asset's price becomes large.
+
+```go
+func BeginBlocker(ctx sdk.Context, k keeper.Keeper) {
+    if !validateTotalBorrows(ctx, k) {
+        k.PauseNewLoans(ctx)
+    }
+}
+
+func validateTotalBorrows(ctx sdk.Context, k keeper.Keeper) {
+    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()) {
+            return false
+        }
+    }
+    return true
+}
+```
+
+## 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
+
+## 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)
+- [Setting invalid parameters in `x/distribution` module causes `panic` in `BeginBlocker`](https://github.com/cosmos/cosmos-sdk/issues/5808). Valid parameters are [described in the documentation](https://docs.cosmos.network/v0.45/modules/distribution/07_params.html).

not-so-smart-contracts/cosmos/broken_bookkeeping/README.md

@@ -0,0 +1,71 @@
+# 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.
+
+## 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.
+
+```go
+func BalanceInvariant(k Keeper) sdk.Invariant {
+    return func(ctx sdk.Context) (string, bool) {
+        weAreFine := true
+        msg := "hodling hard"
+
+        weHold := k.bankKeeper.SpendableCoins(authtypes.NewModuleAddress(types.ModuleName)).AmountOf("BTC")
+        usersDeposited := k.GetTotalDeposited("BTC")
+
+        if weHold != usersDeposited {
+            msg = fmt.Sprintf("%dBTC missing! Halting chain.\n", usersDeposited - weHold)
+            weAreFine = false
+        }
+
+        return sdk.FormatInvariant(types.ModuleName, "hodl-balance",), weAreFine
+    }
+}
+```
+
+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`.
+
+Because an invariant's failure stops the chain, the bug constitutes 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)`.
+
+Implementation of the `GetExchangeRate` method computing an exchange rate is presented below. 
+
+```go
+func (k Keeper) GetExchangeRate(tokenDenom string) sdk.Coin {
+    uTokenDenom := createUDenom(tokenDenom)
+
+    tokensHeld := k.bankKeeper.SpendableCoins(authtypes.NewModuleAddress(types.ModuleName)).AmountOf(tokenDenom).ToDec()
+    tokensBorrowed := k.GetTotalBorrowed(tokenDenom)
+    uTokensInCirculation := k.bankKeeper.GetSupply(uTokenDenom).Amount
+
+    return (tokensHeld + tokensBorrowed) / uTokensInCirculation
+}
+
+```
+
+A malicious user can screw an 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
+
+The first "attack" could be pulled of by sending [`MsgSend`](https://docs.cosmos.network/main/modules/bank/03_messages.html#msgsend) message. However, it would be not profitable (probably), as executing it would irreversibly decrease an 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.
+
+
+## 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)
+
+## 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).
+- [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))
+

not-so-smart-contracts/cosmos/incorrect_getsigners/README.md

@@ -0,0 +1,89 @@
+# Incorrect Signers
+
+In Cosmos, transaction's signature(s) are validated against public keys (addresses) taken from the transaction itself,
+where locations of the keys [are specified in `GetSigners` methods](https://docs.cosmos.network/v0.46/core/transactions.html#signing-transactions).
+
+In the simplest case there is just one signer required, and its address is simple to use correctly.
+However, in more complex scenarios like when multiple signatures are required or a delegation schema is implemented,
+it is possible to make mistakes about what addresses in the transaction (the message) are actually authenticated. 
+
+Fortunately, mistakes in `GetSigners` should make part of application's intended functionality not working,
+making it easy to spot the bug.  
+
+## Example 
+
+The example application allows an author to create posts. A post can be created with a `MsgCreatePost` message, which has `signer` and `author` fields.
+
+```proto
+service Msg {
+      rpc CreatePost(MsgCreatePost) returns (MsgCreatePostResponse);
+}
+
+message MsgCreatePost {
+  string signer = 1;
+  string author = 2;
+  string title = 3;
+  string body = 4;
+}
+
+message MsgCreatePostResponse {
+  uint64 id = 1;
+}
+
+message Post {
+  string author = 1;
+  uint64 id = 2;
+  string title = 3;
+  string body = 4;
+}
+```
+
+The `signer` field is used for signature verification - as can be seen in `GetSigners` method below.
+
+```go
+func (msg *MsgCreatePost) GetSigners() []sdk.AccAddress {
+    signer, err := sdk.AccAddressFromBech32(msg.Signer)
+    if err != nil {
+        panic(err)
+    }
+    return []sdk.AccAddress{Signer}
+}
+
+func (msg *MsgCreatePost) GetSignBytes() []byte {
+    bz := ModuleCdc.MustMarshalJSON(msg)
+    return sdk.MustSortJSON(bz)
+}
+
+func (msg *MsgCreatePost) ValidateBasic() error {
+    _, err := sdk.AccAddressFromBech32(msg.Signer)
+    if err != nil {
+        return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
+    }
+    return nil
+}
+```
+
+The `author` field is saved along with the post's content:
+
+```go
+func (k msgServer) CreatePost(goCtx context.Context, msg *types.MsgCreatePost) (*types.MsgCreatePostResponse, error) {
+    ctx := sdk.UnwrapSDKContext(goCtx)
+
+    var post = types.Post{
+        Author: msg.Author,
+        Title:  msg.Title,
+        Body:   msg.Body,
+    }
+
+    id := k.AppendPost(ctx, post)
+
+    return &types.MsgCreatePostResponse{Id: id}, nil
+}
+```
+
+The bug here - mismatch between the message signer address and the stored address - allows users to impersonate other users by sending an arbitrary `author` field.
+
+## Mitigations
+
+- Keep signers-related logic simple
+- Implement basic sanity tests for all functionalities

not-so-smart-contracts/cosmos/messages_priority/README.md

@@ -0,0 +1,62 @@
+# Not prioritized messages
+
+Some message types may be more important than others and should have priority over them. That is, the more significant a message type is, the more quickly it should be included in a block, before other messages are.
+
+Failing to prioritize message types allows attackers to front-run them, possibly gaining unfair advantage. Moreover, during high network congestion, the message may be simply not included in a block for a long period, causing the system to malfunction.
+
+In the Cosmos's mempool, transactions are [ordered in first-in-first-out (FIFO) manner](https://github.com/tendermint/tendermint/blob/f9e0f77af333f4ab7bfa1c0c303f7db47cec0c9e/docs/architecture/adr-067-mempool-refactor.md#context) by default. Especially, there is no fee-based ordering.
+
+## Example
+
+An example application implements a lending platform. It uses a price oracle mechanism, where privileged entities can vote on new assets' prices. The mechanism is implemented as standard messages.
+
+```go
+service Msg {
+  rpc Lend(MsgLend) returns (MsgLendResponse);
+
+  rpc Borrow(MsgBorrow) returns (MsgBorrowResponse);
+
+  rpc Liquidate(MsgLiquidate) returns (MsgLiquidateResponse);
+
+  rpc OracleCommitPrice(MsgOracleCommitPrice) returns (MsgOracleCommitPriceResponse);
+
+  rpc OracleRevealPrice(MsgOracleRevealPrice) returns (MsgOracleRevealPriceResponse);
+}
+```
+
+Prices ought to be updated (committed and revealed) after every voting period. However, an attacker can spam the network with low-cost transactions to completely fill blocks, leaving no space for price updates. He can then profit from the fact that the system uses outdated, stale prices.
+
+## Example 2
+
+Lets consider a liquidity pool application that implements the following message types:
+
+```go
+service Msg {
+  rpc CreatePool(MsgCreatePool) returns (MsgCreatePoolResponse);
+
+  rpc Deposit(MsgDeposit) returns (MsgDepositResponse);
+
+  rpc Withdraw(MsgWithdraw) returns (MsgWithdrawResponse);
+
+  rpc Swap(MsgSwap) returns (MsgSwapResponse);
+
+  rpc Pause(MsgPause) returns (MsgPauseResponse);
+
+  rpc Resume(MsgResume) returns (MsgResumeResponse);
+}
+```
+
+There is the `Pause` message, which allows privileged users to stop the pool.
+
+Once a bug in pool's implementation is discovered, attackers and the pool's operators will compete for whose message is first executed (`Swap` vs `Pause`). Prioritizing `Pause` messages will help pool's operators to prevent exploitation, but in this case it won't stop the attackers completely. They can outrun the `Pause` message by order of magnitude - so the priority will not matter -  or even cooperate with a malicious validator node - who can order his mempool in an arbitrary way.
+
+
+## Mitigations
+
+- [Use `CheckTx`'s `priority` return value](https://github.com/tendermint/spec/blob/v0.7.1/spec/abci/abci.md#checktx-1) to prioritize messages. Please note that this feature has a transaction (not a message) granularity - users can send multiple messages in a single transaction, and it is the transaction that will have to be prioritized.
+- Perform authorization for prioritized transactions as early as possible. That is, during the `CheckTx` phase. This will prevent attackers from filling whole blocks with invalid, but prioritized transactions. In other words, implement a mechanism that will prevent validators from accepting not-authorized, prioritized messages into a mempool.
+- Alternatively, charge a high fee for prioritized transactions to disincentivize attackers.
+
+## External examples
+- [Terra Money's oracle messages were not prioritized](https://cryptorisks.substack.com/p/ust-december-2021) (search for "priority"). It was [fixed with modifications to Tendermint](https://github.com/terra-money/tendermint/commit/6805b4866bdbd6933000eb0e761acbf15edd8ed6).
+- [Umee oracle and orchestrator messages were not prioritized](https://github.com/trailofbits/publications/blob/master/reviews/Umee.pdf) (search for finding TOB-UMEE-20 and TOB-UMEE-31).