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

Unexpected Balances #63

Closed
wants to merge 13 commits into from
Closed

Conversation

indeqs
Copy link
Contributor

@indeqs indeqs commented Jun 2, 2024

Related Issue

Checklist

Describe the changes you've made:

Added force-feeding as a technique used by attackers to send Ether directly to a smart contract address without invoking any of its functions. This can disrupt the contract's internal accounting mechanisms and logic, particularly if the contract relies on the assumption that its balance can only change through controlled transactions.

Type of change

Select the appropriate checkbox:

  • Bug fix (fixing an issue with existing vulnerability data)
  • [✅] New feature (adding a new vulnerability or category)
  • Documentation update (improving existing information)

Copy link
Contributor

@rakesh0x7 rakesh0x7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @indeqs,

Could you please review the commit history as you added the commit 3bcd60b to this pull request (PR)? However, it's important to note that this commit actually belongs to PR #58.

@rakesh0x7 rakesh0x7 mentioned this pull request Jun 4, 2024
4 tasks

### Normal Contract Behavior

In typical smart contract operation, Ether is sent to a contract via a transaction that calls a function or invokes the `receive()` or `fallback()` functions. If a contract lacks these functions, transactions sending Ether to it will normally be reverted, ensuring the contract does not inadvertently receive funds.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth also noting that ETH can be sent along with payable functions

@@ -0,0 +1,89 @@
# Force Feeding
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better title would be "Unexpected Ether Balance"? This way it focuses more on the vulnerability rather than the attack vector. Would have to update the below paragraph to reflect this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about "Unexpected Ether Transfer"?

@@ -0,0 +1,109 @@
In Solidity, the `abi.encodePacked()` function is used to create tightly packed byte arrays which can then be hashed using `keccak256()`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted by rakesh, this got accidentally included it seems

Comment on lines +23 to +25
## Preventing Force Feeding

To safeguard against force-feeding, consider the following strategies:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self to come back to reconsider these prevention methods further

@kadenzipfel
Copy link
Owner

Actually, I think this fits best under https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/dos-revert.md#unexpected-balance. Can we instead provide a simplified version of this with the above recommended changes to the dos-revert file?

@indeqs
Copy link
Contributor Author

indeqs commented Jun 6, 2024

@kadenzipfel the file at
https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/dos-revert.md#unexpected-balance
focuses on DoS while this PR focuses on accounting logic that may arise. I don't see it fit to be there henceforth. Thoughts...

@kadenzipfel
Copy link
Owner

@kadenzipfel the file at https://github.com/kadenzipfel/smart-contract-vulnerabilities/blob/master/vulnerabilities/dos-revert.md#unexpected-balance focuses on DoS while this PR focuses on accounting logic that may arise. I don't see it fit to be there henceforth. Thoughts...

Yeah that's a good point. I think the reason I initially removed "forcibly sending ether" was because it was documented as an attack rather than as a vulnerability, but if we're documenting this instead as unexpectedly receiving ether then that's actually good to have imo.

One thing I wonder is whether this should more generally encompass unexpected balances as a whole since the same type of issue could be caused by having an unexpected amount of tokens, e.g. an inflation attack. Then maybe would be ideal to include the ability to forcefully send ether as an odd evm gotcha to be aware of. Wdyt?

@indeqs
Copy link
Contributor Author

indeqs commented Jun 7, 2024

That sounds good. Just to make sure I'm getting you correctly, the focus will be:
unexpected balances in smart contracts may lead to accounting issues when relied upon and examples include "force-feeding a contract" and inflation attacks

Am I getting you right?

@kadenzipfel
Copy link
Owner

That sounds good. Just to make sure I'm getting you correctly, the focus will be: unexpected balances in smart contracts may lead to accounting issues when relied upon and examples include "force-feeding a contract" and inflation attacks

Am I getting you right?

Yep!

README.md Outdated
@@ -6,6 +6,7 @@
- [Timestamp Dependence](./vulnerabilities/timestamp-dependence.md)
- [Authorization Through tx.origin](./vulnerabilities/authorization-txorigin.md)
- [Floating Pragma](./vulnerabilities/floating-pragma.md)
- [Force Feeding](./vulnerabilities/force-feeding.md)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should update the name here, also would be good to change the filename as well

indeqs and others added 5 commits June 10, 2024 23:29
unexpected-balances.md is still a work in progress and not yet ready for a merge. When done, it will supersede force-feeding as discussed in this [PR](kadenzipfel#63)
@indeqs indeqs changed the title Force Feeding Unexpected Balances Jun 13, 2024
@indeqs indeqs closed this Jun 13, 2024
This was referenced Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Force Feeding
3 participants