-
Notifications
You must be signed in to change notification settings - Fork 276
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
Lack of precision vuln updated to precision loss. #86
Open
0xSandyy
wants to merge
13
commits into
kadenzipfel:master
Choose a base branch
from
0xSandyy:precision-loss
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
d034311
msg.value in a loop vulnerabilitiy added
0xSandyy 441118f
Vulnerability Updated 06/09
0xSandyy 6dee120
Vulnerability Updated: Fixed indentation and other minor changes.
0xSandyy db0995e
Listing added for the vulnerability
0xSandyy e77623c
msgValueLoop vuln updated: added requested changes
0xSandyy 692cf8a
Lack of precision vuln updated to precision loss
0xSandyy d255265
Merge branch 'master' into msgValue
kadenzipfel 5816d4f
precision loss vuln updated: added requested changes
0xSandyy ca935da
merging precison loss
0xSandyy 6d54a42
Merge branch 'msgValue' into precision-loss
0xSandyy e394118
Lack of precision vuln updated to precision loss
0xSandyy 06a6c42
rebased and dropped msgValue branch
0xSandyy 6609011
Update README.md
kadenzipfel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file was deleted.
Oops, something went wrong.
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
# Using ``msg.value`` in a Loop | ||
|
||
The value of ``msg.value`` in a transaction’s call never gets updated, even if the called contract ends up sending some or all of the ETH to another contract. This means that using ``msg.value`` in ``for`` or ``while`` loops, without extra accounting logic, will either lead to the transaction reverting (when there are no longer sufficient funds for later iterations), or to the contract being drained (when the contract itself has an ETH balance). | ||
|
||
```solidity | ||
contract depositer { | ||
function deposit(address weth) payable external { | ||
for (uint i = 0; i < 5; i ++) { | ||
WETH(weth).deposit{value: msg.value}(); | ||
} | ||
} | ||
} | ||
``` | ||
In the above example, first iteration will use all the ``msg.value`` for the external call and all other iterations can: | ||
- Drain the contract if enough ETH balance exists inside the contract to cover all the iterations. | ||
- Revert if enough ETH balance doesn't exist inside the contract to cover all the iterations. | ||
- Succeed if the external implementation succeeds with zero value transfers. | ||
|
||
Also, if a function has a check like ``require(msg.value == 1e18, "Not Enough Balance")``, that function can be called multiple times in a same transaction by sending ``1 ether`` once as ``msg.value`` is not updated in a transaction call. | ||
|
||
```solidity | ||
function batchBuy(address[] memory addr) external payable{ | ||
mapping (uint => address) nft; | ||
|
||
for (uint i = 0; i < addr.length; i++) { | ||
buy1NFT(addr[i]) | ||
} | ||
|
||
function buy1NFT(address to) internal { | ||
if (msg.value < 1 ether) { // buy unlimited times after sending 1 ether once | ||
revert("Not enough ether"); | ||
} | ||
nft[numero] = address; | ||
} | ||
} | ||
``` | ||
|
||
Thus, using ``msg.value`` inside a loop is dangerous because this might allow the sender to ``re-use`` the ``msg.value``. | ||
|
||
Reuse of ``msg.value`` can also show up with payable multicalls. Multicalls enable a user to submit a list of transactions to avoid paying the 21,000 gas transaction fee over and over. However, If ``msg.value`` gets ``re-used`` while looping through the functions to execute, it can cause a serious issue like the [Opyn Hack](https://peckshield.medium.com/opyn-hacks-root-cause-analysis-c65f3fe249db). | ||
|
||
## Sources | ||
- https://www.rareskills.io/post/smart-contract-security#:~:text=Using%20msg.,show%20up%20with%20payable%20multicalls. | ||
- https://trustchain.medium.com/ethereum-msg-value-reuse-vulnerability-5afd0aa2bcef |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
## Precision Loss | ||
|
||
Solidity uses fixed point arithmetic, that means it doesn't support decimal value. As a result, any decimal value is truncated. Thus, performing numerical operations can cause precision loss especially when division is performed before multiplication. | ||
|
||
```solidity | ||
uint a = 11; | ||
uint b = 2; | ||
uint c = 10; | ||
|
||
function funcA() public { | ||
return a / b * c; | ||
} | ||
|
||
function funcB() public { | ||
return a * c / b; | ||
} | ||
``` | ||
|
||
In the above code example, ``funcA()`` returns: | ||
```soldiity | ||
11 / 2 * 10 = 50 | ||
``` | ||
and ``funcB()`` returns: | ||
```solidity | ||
11 * 10 / 2 = 55 | ||
``` | ||
Solidity truncates any non-integer result to the nearest lower integer. Thus in ``funcA()``, instead of ``11 / 2 = 5.5``, ``0.5`` is truncated and the result amounts to ``5``. This results in the function returning ``50`` instead of ``55`` due to division before multiplication whereas multiplication before division returns the correct value. | ||
|
||
Also, if the Numerator is lower than the Denominator during division, the result will be zero in solidity. | ||
|
||
```solidity | ||
uint num = 10; | ||
uint den = 20; | ||
|
||
function foo() public pure returns(uint result) { | ||
result = num / den; // returns 0 | ||
} | ||
``` | ||
In regular math, the function ``foo()`` will return ``10 / 20 = 0.5``. But, the function returns ``0`` due to integer truncation in solidity. Therefore, It needs to be always made sure that numerator is greater than the denominator to avoid unexpected results. |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to merge/rebase to remove this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's still included for some reason. Just me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no changes in the latest commit for msgvalue-loop.md file but file changes shows msgvalue-loop.md file being updated.
Also, Nothing seems to work even after merging and rebasing with both msgValue and master branch. I think it is because one commit from msgValue got included here and it is causing the problem. As this PR has no conflicts with the base branch, and msgvalue-loop.md content hasn't been changed and is similar to the master branch, it can be merged without any issues.
Or we can close this PR and create a new one.