diff --git a/not-so-smart-contracts/cosmos/README.md b/not-so-smart-contracts/cosmos/README.md index 1f8cad44..768aaf9b 100644 --- a/not-so-smart-contracts/cosmos/README.md +++ b/not-so-smart-contracts/cosmos/README.md @@ -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. diff --git a/not-so-smart-contracts/cosmos/abci_fast/README.md b/not-so-smart-contracts/cosmos/abci_fast/README.md index da11a4e6..443834ba 100644 --- a/not-so-smart-contracts/cosmos/abci_fast/README.md +++ b/not-so-smart-contracts/cosmos/abci_fast/README.md @@ -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) { @@ -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). diff --git a/not-so-smart-contracts/cosmos/abci_panic/README.md b/not-so-smart-contracts/cosmos/abci_panic/README.md index 0e9757f3..6019c655 100644 --- a/not-so-smart-contracts/cosmos/abci_panic/README.md +++ b/not-so-smart-contracts/cosmos/abci_panic/README.md @@ -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) { @@ -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 } } @@ -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) diff --git a/not-so-smart-contracts/cosmos/broken_bookkeeping/README.md b/not-so-smart-contracts/cosmos/broken_bookkeeping/README.md index 7907e6c9..dbf2d952 100644 --- a/not-so-smart-contracts/cosmos/broken_bookkeeping/README.md +++ b/not-so-smart-contracts/cosmos/broken_bookkeeping/README.md @@ -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 { @@ -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 { @@ -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)) diff --git a/not-so-smart-contracts/cosmos/incorrect_getsigners/README.md b/not-so-smart-contracts/cosmos/incorrect_getsigners/README.md index 9660059f..3dad9d56 100644 --- a/not-so-smart-contracts/cosmos/incorrect_getsigners/README.md +++ b/not-so-smart-contracts/cosmos/incorrect_getsigners/README.md @@ -1,18 +1,14 @@ -# Incorrect Signers +# Addressing 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 Cosmos, a transaction's signatures are validated against public keys (addresses) extracted from the transaction itself, with the key locations [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. +For simple cases where only one signer is needed, using the correct address is straightforward. However, in more complex scenarios involving multiple signatures or delegation schemas, it's possible to make mistakes in determining which addresses in the transaction (the message) are being authenticated. -Fortunately, mistakes in `GetSigners` should make part of application's intended functionality not working, -making it easy to spot the bug. +Thankfully, errors in `GetSigners` typically result in a portion of the application's intended functionality not working, making it easier to identify the issue. ## 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. +Consider an example application that allows authors to create posts. A post can be created using a `MsgCreatePost` message, which includes `signer` and `author` fields. ```proto service Msg { @@ -38,7 +34,7 @@ message Post { } ``` -The `signer` field is used for signature verification - as can be seen in `GetSigners` method below. +The `signer` field is used for signature verification, as demonstrated in the `GetSigners` method below. ```go func (msg *MsgCreatePost) GetSigners() []sdk.AccAddress { @@ -63,7 +59,7 @@ func (msg *MsgCreatePost) ValidateBasic() error { } ``` -The `author` field is saved along with the post's content: +The `author` field is stored along with the post content: ```go func (k msgServer) CreatePost(goCtx context.Context, msg *types.MsgCreatePost) (*types.MsgCreatePostResponse, error) { @@ -81,9 +77,9 @@ func (k msgServer) CreatePost(goCtx context.Context, msg *types.MsgCreatePost) ( } ``` -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. +In this example, the bug involves a mismatch between the message signer address and the stored address, allowing users to impersonate others by providing an arbitrary `author` field. -## Mitigations +## Mitigation Strategies -- Keep signers-related logic simple -- Implement basic sanity tests for all functionalities +- Keep the logic related to signers simple. +- Implement basic sanity tests for all functionalities. diff --git a/not-so-smart-contracts/cosmos/messages_priority/README.md b/not-so-smart-contracts/cosmos/messages_priority/README.md index ef7c0fa6..8f22e63a 100644 --- a/not-so-smart-contracts/cosmos/messages_priority/README.md +++ b/not-so-smart-contracts/cosmos/messages_priority/README.md @@ -1,14 +1,14 @@ -# Not prioritized messages +# Prioritizing 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. +Some message types may be more important than others and should have priority over them. In other words, the more significant a message type is, the more quickly it should be included in a block, before other messages. -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. +Failing to prioritize message types enables attackers to front-run them, potentially gaining an unfair advantage. Additionally, during high network congestion, the message may not be included in a block for an extended 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. +By default, transactions in Cosmos's mempool are [ordered in a first-in-first-out (FIFO) manner](https://github.com/tendermint/tendermint/blob/f9e0f77af333f4ab7bfa1c0c303f7db47cec0c9e/docs/architecture/adr-067-mempool-refactor.md#context). Notably, 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. +Consider an application that 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 { @@ -24,11 +24,11 @@ service Msg { } ``` -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. +Prices should 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. The attacker 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: +Consider a liquidity pool application that implements the following message types: ```go service Msg { @@ -46,17 +46,17 @@ service Msg { } ``` -There is the `Pause` message, which allows privileged users to stop the pool. +The `Pause` message 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. +When a bug is discovered in the pool's implementation, attackers and the pool's operators compete to determine whose message is executed first (`Swap` vs `Pause`). Prioritizing `Pause` messages helps pool operators prevent exploitation, but in this case, it doesn't completely stop the attackers. They can outrun the `Pause` message by an order of magnitude, rendering the priority irrelevant or even collaborate with a malicious validator node, which can order its mempool arbitrarily. ## 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. +- [Use the `CheckTx`'s `priority` return value](https://github.com/tendermint/spec/blob/v0.7.1/spec/abci/abci.md#checktx-1) to prioritize messages. Note that this feature has transaction (not message) granularity: users can send multiple messages in a single transaction, and the transaction must be prioritized. +- Perform authorization for prioritized transactions as early as possible, preferably during the `CheckTx` phase. This prevents attackers from filling entire blocks with invalid but prioritized transactions. In other words, implement a mechanism that prevents validators from accepting unauthorized, prioritized messages into a mempool. - Alternatively, charge a high fee for prioritized transactions to disincentivize attackers. -## External examples +## 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). +- [Terra Money's oracle messages were not prioritized](https://cryptorisks.substack.com/p/ust-december-2021) (search for "priority"). The issue was [resolved 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). diff --git a/not-so-smart-contracts/cosmos/missing_error_handler/README.md b/not-so-smart-contracts/cosmos/missing_error_handler/README.md index 72a4e93f..b60ce574 100644 --- a/not-so-smart-contracts/cosmos/missing_error_handler/README.md +++ b/not-so-smart-contracts/cosmos/missing_error_handler/README.md @@ -1,10 +1,10 @@ -# Missing error handler +# Missing Error Handler -The idiomatic way of handling errors in `Go` is to compare the returned error to nil. This way of checking for errors gives the programmer a lot of control. However, when error handling is ignored it can also lead to numerous problems. The impact of this is most obvious in method calls in the `bankKeeper` module, which even causes some accounts with insufficient balances to perform `SendCoin` operations normally without triggering a transaction failure. +The idiomatic way of handling errors in `Go` involves comparing the returned error to nil. This error-checking method provides the programmer with a significant level of control. However, ignoring error handling can result in various issues. The effects are particularly evident in method calls in the `bankKeeper` module, where some accounts with insufficient balances still carry out `SendCoin` operations normally, without triggering a transaction failure. ## Example -In the following code, `k.bankKeeper.SendCoins(ctx, sender, receiver, amount)` does not have any return values being used, including `err`. This results in `SendCoin` not being able to prevent the transaction from executing even if there is an `error` due to insufficient balance in `SendCoin`. +In the code snippet below, `k.bankKeeper.SendCoins(ctx, sender, receiver, amount)` does not utilize any return values, including `err`. Consequently, `SendCoin` cannot prevent the transaction from executing, even if there is an `error` caused by an insufficient balance in `SendCoin`. ```golang func (k msgServer) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { @@ -17,11 +17,11 @@ func (k msgServer) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*typ ## Mitigations -- Implement the error handling process instead of missing it +- Implement the error handling process, rather than omitting it. -## External examples +## External Examples -- [ignite's tutorials](https://github.com/ignite/cli/issues/2828). +- [Ignite's Tutorials](https://github.com/ignite/cli/issues/2828) - [Fadeev's Loan Project](https://github.com/fadeev/loan/blob/master/x/loan/keeper/msg_server_approve_loan.go) -- [JackalLabs](https://github.com/JackalLabs/canine-chain/issues/8). -- [OllO](https://github.com/OllO-Station/ollo/issues/20) +- [JackalLabs](https://github.com/JackalLabs/canine-chain/issues/8) +- [OllO Station](https://github.com/OllO-Station/ollo/issues/20) diff --git a/not-so-smart-contracts/cosmos/non_determinism/README.md b/not-so-smart-contracts/cosmos/non_determinism/README.md index ab44be24..f01d57a4 100644 --- a/not-so-smart-contracts/cosmos/non_determinism/README.md +++ b/not-so-smart-contracts/cosmos/non_determinism/README.md @@ -1,7 +1,6 @@ # Non-determinism -Non-determinism in conensus-relevant code will cause the blockchain to halt. -There are quite a few sources of non-determinism, some of which are specific to the Go language: +Non-determinism in consensus-relevant code can cause the blockchain to halt. Various sources of non-determinism exist, with some specific to the Go language: - [`range` iterations over an unordered map or other operations involving unordered structures](https://lev.pm/posts/2020-04-18-golang-map-randomness/) - [Implementation (platform) dependent types like `int`](https://go.dev/ref/spec#Numeric_types) or `filepath.Ext` @@ -14,7 +13,7 @@ There are quite a few sources of non-determinism, some of which are specific to ## Example -Below we can see an iteration over a `amounts` `map`. If `k.GetPool` fails for more than one `asset`, then different nodes will fail with different errors, causing the chain to halt. +The following example demonstrates an iteration over a `map` of `amounts`. If `k.GetPool` fails for more than one `asset`, different nodes will produce different errors, causing the chain to halt. ```go func (k msgServer) CheckAmounts(goCtx context.Context, msg *types.MsgCheckAmounts) (*types.MsgCheckAmountsResponse, error) { @@ -42,15 +41,15 @@ func (k msgServer) CheckAmounts(goCtx context.Context, msg *types.MsgCheckAmount } ``` -Even if we fix the `map` problem, it is still possible that the `total` overflows for nodes running on 32-bit architectures earlier than for the rest of the nodes, again causing the chain split. +Even after fixing the `map` issue, the `total` overflow may occur for nodes running on 32-bit architectures earlier than others, causing a chain split. ## Mitigations - Use static analysis, for example [custom CodeQL rules](https://github.com/crypto-com/cosmos-sdk-codeql) - Test your application with nodes running on various architectures or require nodes to run on a specific one -- Prepare and test procedures for recovering from a blockchain split +- Develop and test procedures for recovering from a blockchain split ## External examples -- [ThorChain halt due to "iteration over a map error-ing at different indexes"](https://gitlab.com/thorchain/thornode/-/issues/1169) -- [Cyber's had problems with `float64` type](https://github.com/cybercongress/go-cyber/issues/66) +- [ThorChain halted due to "iteration over a map error-ing at different indexes"](https://gitlab.com/thorchain/thornode/-/issues/1169) +- [Cyber encountered problems with the `float64` type](https://github.com/cybercongress/go-cyber/issues/66) diff --git a/not-so-smart-contracts/cosmos/rounding_errors/README.md b/not-so-smart-contracts/cosmos/rounding_errors/README.md index dd9e908a..9a58b3e5 100644 --- a/not-so-smart-contracts/cosmos/rounding_errors/README.md +++ b/not-so-smart-contracts/cosmos/rounding_errors/README.md @@ -1,17 +1,17 @@ -# Rounding errors +# Rounding Errors -Application developers must take care of correct rounding of numbers, especially if the rounding impacts tokens amounts. +Application developers must pay attention to the correct rounding of numbers, particularly if the rounding affects token amounts. -Cosmos-sdk offers two custom types for dealing with numbers: +Cosmos-sdk provides two custom types for handling numbers: - `sdk.Int` (`sdk.UInt`) type for integral numbers - `sdk.Dec` type for decimal arithmetic -The `sdk.Dec` type [has problems with precision and does not guarantee associativity](https://github.com/cosmos/cosmos-sdk/issues/7773), so it must be used carefully. But even if a more robust library for decimal numbers is deployed in the cosmos-sdk, rounding may be unavoidable. +The `sdk.Dec` type [has issues with precision and does not guarantee associativity](https://github.com/cosmos/cosmos-sdk/issues/7773), so it must be used cautiously. However, even if a more robust library for decimal numbers is implemented in the cosmos-sdk, rounding might still be unavoidable. ## Example -Below we see a simple example demonstrating `sdk.Dec` type's precision problems. +The simple example below demonstrates the precision problems with the `sdk.Dec` type. ```go func TestDec() { @@ -29,12 +29,12 @@ func TestDec() { ## Mitigations -- Ensure that all tokens operations that must round results always benefit the system (application) and not users. In other words, always decide on the correct rounding direction. See [Appendix G in the Umee audit report](https://github.com/trailofbits/publications/blob/master/reviews/Umee.pdf) +- Ensure that all token operations requiring rounded results always benefit the system (application) and not the users. In other words, consistently choose the correct rounding direction. See [Appendix G in the Umee audit report](https://github.com/trailofbits/publications/blob/master/reviews/Umee.pdf). -- Apply "multiplication before division" pattern. That is, instead of computing `(x / y) * z` do `(x * z) / y` +- Use the "multiplication before division" pattern. In other words, instead of computing `(x / y) * z`, perform `(x * z) / y`. -- Observe [issue #11783](https://github.com/cosmos/cosmos-sdk/issues/11783) for a replacement of the `sdk.Dec` type +- Monitor [issue #11783](https://github.com/cosmos/cosmos-sdk/issues/11783) for a replacement of the `sdk.Dec` type. -## External examples +## External Examples -- [Umee had vulnerability caused by incorrect rounding direction](https://github.com/umee-network/umee/issues/545) +- [Umee experienced a vulnerability caused by an incorrect rounding direction](https://github.com/umee-network/umee/issues/545). diff --git a/not-so-smart-contracts/cosmos/unregistered_msg_handler/README.md b/not-so-smart-contracts/cosmos/unregistered_msg_handler/README.md index 5184400f..4269daf1 100644 --- a/not-so-smart-contracts/cosmos/unregistered_msg_handler/README.md +++ b/not-so-smart-contracts/cosmos/unregistered_msg_handler/README.md @@ -1,12 +1,12 @@ -# Unregistered message handler +# Unregistered Message Handler -In the legacy version of the `Msg Service`, all messages have to be registered in a module keeper's `NewHandler` method. Failing to do so would prevent users from sending the not-registered message. +In the legacy version of the `Msg Service`, every message must be registered within a module keeper's `NewHandler` method. Failure to register a message would prevent users from sending the unregistered message. -In [the recent Cosmos version manual registration is no longer needed](https://docs.cosmos.network/v0.47/building-modules/msg-services). +In [the recent Cosmos version, manual registration is no longer needed](https://docs.cosmos.network/v0.47/building-modules/msg-services). ## Example -There is one message handler missing. +In the code below, one message handler is missing. ```go service Msg { @@ -98,85 +98,17 @@ func NewHandler(k keeper.Keeper) sdk.Handler { case *types.MsgUpdateUserAddress: res, err := msgServer.UpdateUserAddress(sdk.WrapSDKContext(ctx), msg) return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgUpdateCall: - res, err := msgServer.UpdateCall(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSendBatch: - res, err := msgServer.SendBatch(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgCancelUserAddress: - res, err := msgServer.CancelUserAddress(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgRequestBatch: - res, err := msgServer.RequestBatch(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgUpdateEthClaim: - res, err := msgServer.UpdateEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSendCall: - res, err := msgServer.SendCall(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSetCall: - res, err := msgServer.SetCall(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgCancelEthClaim: - res, err := msgServer.CancelEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgConfirmEthClaim: - res, err := msgServer.ConfirmEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgConfirmCall: - res, err := msgServer.ConfirmCall(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgRequestCall: - res, err := msgServer.RequestCall(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgConfirmUserAddress: - res, err := msgServer.ConfirmUserAddress(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgRequestUserAddress: - res, err := msgServer.RequestUserAddress(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSendEthClaim: - res, err := msgServer.SendEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSetEthClaim: - res, err := msgServer.SetEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgCancelBatch: - res, err := msgServer.CancelBatch(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSetUserAddress: - res, err := msgServer.SetUserAddress(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgRequestEthClaim: - res, err := msgServer.RequestEthClaim(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgConfirmBatch: - res, err := msgServer.ConfirmBatch(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgUpdateBatch: - res, err := msgServer.UpdateBatch(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - case *types.MsgSendUserAddress: - res, err := msgServer.SendUserAddress(sdk.WrapSDKContext(ctx), msg) - return sdk.WrapServiceResult(ctx, res, err) - - default: - return nil, sdkerrors.Wrap(sdkerrors.ErrUnknownRequest, fmt.Sprintf("Unrecognized Gravity Msg type: %v", msg.Type())) - } - } -} +... ``` -And it is the `CancelCall` msg. +The missing message is the `CancelCall` msg. ## Mitigations -- Use the recent Msg Service mechanism -- Test all functionalities -- Deploy static-analysis tests in CI pipeline for all manually maintained code that must be repeated in multiple files/methods +- Use the recent Msg Service mechanism. +- Test all functionalities. +- Deploy static-analysis tests in the CI pipeline for all manually maintained code that must be repeated in multiple files/methods. ## External examples -- The bug occured in the [Gravity Bridge](https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/64). It was impossible to send evidence of malicious behavior, which impacted Gravity Bridge's security model. +- The bug occurred in the [Gravity Bridge](https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/64). It was impossible to send evidence of malicious behavior, which impacted Gravity Bridge's security model.