[WIP] Not-so-smart pallets #124
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Added a pallet that does not comply with Verify first, write last principle. 2. Added a pallet that has an integer overflow / underflow 3. Added README.md for both pallets with almost no content
1. Renamed transfer.rs to pallet-overflow.rs to comply with substrate naming convention 2. pallet-overflow.rs will compile in substrate-node-template and the bug is exploitable
TotalSupplyDefaultValue is not the actual total supply but instead the default one. So changed comment to note that accordingly.
This pallet is no longer insecure after polkadot-v0.9.25. However, it is still considered best practice so I'm including it in here.
This pallet demonstrates the difficulty in creating proper weight functions. If the length of useful_amounts is zero, the weight for DoWork is zero. This also demonstrates the need for benchmarking.
Decided to add an error type so that `ensure!()` can be used to cause the out-of-bounds check
montyly
suggested changes
Aug 17, 2022
|
|
||
| Weights and transaction fees are the two main ways to regulate the consumption of blockchain resources. The overuse of blockchain resources can allow a malicious actor to spam the network to cause a denial-of-service (DoS). Weights are used to manage the time it takes to validate the block. The larger the weight, the more "resources" / time the computation takes. Transaction fees provide an economic incentive to limit the number of resources used to perform operations; the fee for a given transaction is a function of the weight required by the transaction. | ||
|
|
||
| Weights can be fixed or a custom "weight annotation / function" can be implemented. A weight function can calculate the weight, for example, based on the number of database read / writes and the size of the input paramaters (e.g. a long `Vec<>`). To optimize the weight such that users do not pay too little or too much for a transaction, benchmarking can be used to empirically determine the correct weight in worst case scenarios. |
Contributor
There was a problem hiding this comment.
Similar to the other issue, we explain how it works, not what the issue is
|
|
||
| However, if the length of the `useful_amounts` vector is zero, the weight to call `do_work` would be zero. A weight of zero implies that calling this function is financially cheap. This opens the opportunity for an attacker to call `do_work` a large number of times to saturate the network with malicious transactions without having to pay a large fee and could cause a DoS attack. | ||
|
|
||
| One potential fix for this is to set a fixed weight if the length of `useful_amounts` is zero. |
Contributor
There was a problem hiding this comment.
or a static fee? Like static fee + len * price. This would reduce the complexity by having no branching condition
Contributor
Author
There was a problem hiding this comment.
the issue with this is that if len == 1 then you have to pay static_fee * 2 where i specified the weight function to be static_fee * length. Although the branching is not ideal, I think it represents the fix a bit more clearly. what do you think?
There was a problem hiding this comment.
I'm also leaning towards branching, even if it's not the best looking solution.
|
@anishnaik pallets lgtm |
0xicingdeath
approved these changes
Oct 28, 2022
not-so-smart-contracts/substrate/dont_panic/pallet-dont-panic.rs
Outdated
Show resolved
Hide resolved
|
|
||
| However, if the length of the `useful_amounts` vector is zero, the weight to call `do_work` would be zero. A weight of zero implies that calling this function is financially cheap. This opens the opportunity for an attacker to call `do_work` a large number of times to saturate the network with malicious transactions without having to pay a large fee and could cause a DoS attack. | ||
|
|
||
| One potential fix for this is to set a fixed weight if the length of `useful_amounts` is zero. |
There was a problem hiding this comment.
I'm also leaning towards branching, even if it's not the best looking solution.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.