From d0343117ddfbfdc229815518caf0fc5df3384d36 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Thu, 6 Jun 2024 23:08:34 +0545 Subject: [PATCH 01/11] msg.value in a loop vulnerabilitiy added --- vulnerabilities/msgvalue-loop.md | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 vulnerabilities/msgvalue-loop.md diff --git a/vulnerabilities/msgvalue-loop.md b/vulnerabilities/msgvalue-loop.md new file mode 100644 index 0000000..42837b6 --- /dev/null +++ b/vulnerabilities/msgvalue-loop.md @@ -0,0 +1,36 @@ +# Using ``msg.value`` in a loop + +The particularity with msg.value is that ``msg.value`` is set on every contract call and not on the transaction level. Thus, if the function with loop is transferring ``msg.value`` on every iteration, all the ``msg.value`` sent to the function is transferred in the first iteration and other iteration will proceed with ``msg.value = 0`` which may cause unexpected consequences. + +```solidity +contract depositer { + function deposit(address weth) payable external { + for (uint i = 0; i < 5; i ++) { + WETH(weth).deposit{value: msg.value}(); // all the ``msg.value`` will be used in the first iteration + } + } +} +``` + +Also, if a function has a check like ``msg.value > 0.1e18``, that function can be called multiple times in a same transaction as ``msg.value`` value is not subtracted unless it's transferred out of the contract. Thus, Using ``msg.value`` inside a loop is dangerous because this might allow the sender to “re-use” the msg.value. + +```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) { revert("not enough ether") } // buy unlimited times after paying 1 ether once + nft[numero] = address; + } +} +``` + +Reuse of ``msg.value`` can 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, ``msg.value`` gets ``re-used`` while looping through the functions to execute, potentially enabling the user to double spend. + +This was the root cause of 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 \ No newline at end of file From 441118fc7c50600b69903d5d37829c098b92e492 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Sun, 9 Jun 2024 16:56:32 +0545 Subject: [PATCH 02/11] Vulnerability Updated 06/09 --- vulnerabilities/msgvalue-loop.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/vulnerabilities/msgvalue-loop.md b/vulnerabilities/msgvalue-loop.md index 42837b6..85947bf 100644 --- a/vulnerabilities/msgvalue-loop.md +++ b/vulnerabilities/msgvalue-loop.md @@ -1,19 +1,22 @@ -# Using ``msg.value`` in a loop +# Using ``msg.value`` in a loop. -The particularity with msg.value is that ``msg.value`` is set on every contract call and not on the transaction level. Thus, if the function with loop is transferring ``msg.value`` on every iteration, all the ``msg.value`` sent to the function is transferred in the first iteration and other iteration will proceed with ``msg.value = 0`` which may cause unexpected consequences. +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}(); // all the ``msg.value`` will be used in the first iteration + WETH(weth).deposit{value: msg.value}(); } } } ``` +In the above example, first iteration will use all the ``msg.value`` sent and other iterations can: +1. Drain the contract if some ETH balance exists inside the contract. +2. Revert on zero value transfer if ETH balance doesn't exist inside the contract. +3. Succeed with zero value transfer. -Also, if a function has a check like ``msg.value > 0.1e18``, that function can be called multiple times in a same transaction as ``msg.value`` value is not subtracted unless it's transferred out of the contract. Thus, Using ``msg.value`` inside a loop is dangerous because this might allow the sender to “re-use” the msg.value. - +Also, if a function has a check like ``msg.value > 0.1e18``, that function can be called multiple times in a same transaction as ``msg.value`` is not updated in a transaction call. ```solidity function batchBuy(address[] memory addr) external payable{ mapping (uint => address) nft; @@ -27,9 +30,9 @@ function batchBuy(address[] memory addr) external payable{ } ``` -Reuse of ``msg.value`` can 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, ``msg.value`` gets ``re-used`` while looping through the functions to execute, potentially enabling the user to double spend. + Thus, Using ``msg.value`` inside a loop is dangerous because this might allow the sender to ``re-use`` the ``msg.value``. -This was the root cause of the [Opyn Hack](https://peckshield.medium.com/opyn-hacks-root-cause-analysis-c65f3fe249db). +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. From 6dee120faa74d8c9f8610ff7f0f90132a8ed9f27 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Sun, 9 Jun 2024 17:07:17 +0545 Subject: [PATCH 03/11] Vulnerability Updated: Fixed indentation and other minor changes. --- vulnerabilities/msgvalue-loop.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/vulnerabilities/msgvalue-loop.md b/vulnerabilities/msgvalue-loop.md index 85947bf..b7e1834 100644 --- a/vulnerabilities/msgvalue-loop.md +++ b/vulnerabilities/msgvalue-loop.md @@ -16,15 +16,20 @@ In the above example, first iteration will use all the ``msg.value`` sent and ot 2. Revert on zero value transfer if ETH balance doesn't exist inside the contract. 3. Succeed with zero value transfer. -Also, if a function has a check like ``msg.value > 0.1e18``, that function can be called multiple times in a same transaction as ``msg.value`` is not updated in a transaction call. +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) { revert("not enough ether") } // buy unlimited times after paying 1 ether once + if (msg.value < 1 ether) { // buy unlimited times after sending 1 ether once + revert("Not enough ether"); + } nft[numero] = address; } } From db0995e038d8bde490ae85a60b2f3dad992f1235 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Sun, 9 Jun 2024 17:24:30 +0545 Subject: [PATCH 04/11] Listing added for the vulnerability --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 20204af..28e5018 100644 --- a/README.md +++ b/README.md @@ -35,3 +35,4 @@ - [Off-By-One](./vulnerabilities/off-by-one.md) - [Lack of Precision](./vulnerabilities/lack-of-precision.md) - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) +- [Using ``msg.value`` in a loop](./vulnerabilities/msgvalue-loop.md) From e77623c1ea1fb6fe97f8c2d39b030e8c308887bb Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Mon, 10 Jun 2024 10:55:08 +0545 Subject: [PATCH 05/11] msgValueLoop vuln updated: added requested changes --- README.md | 2 +- vulnerabilities/msgvalue-loop.md | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 28e5018..b814488 100644 --- a/README.md +++ b/README.md @@ -35,4 +35,4 @@ - [Off-By-One](./vulnerabilities/off-by-one.md) - [Lack of Precision](./vulnerabilities/lack-of-precision.md) - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) -- [Using ``msg.value`` in a loop](./vulnerabilities/msgvalue-loop.md) +- [Using ``msg.value`` in a Loop](./vulnerabilities/msgvalue-loop.md) diff --git a/vulnerabilities/msgvalue-loop.md b/vulnerabilities/msgvalue-loop.md index b7e1834..55105b7 100644 --- a/vulnerabilities/msgvalue-loop.md +++ b/vulnerabilities/msgvalue-loop.md @@ -1,4 +1,4 @@ -# Using ``msg.value`` in a loop. +# 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). @@ -11,10 +11,10 @@ contract depositer { } } ``` -In the above example, first iteration will use all the ``msg.value`` sent and other iterations can: -1. Drain the contract if some ETH balance exists inside the contract. -2. Revert on zero value transfer if ETH balance doesn't exist inside the contract. -3. Succeed with zero value transfer. +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. @@ -35,7 +35,7 @@ function batchBuy(address[] memory addr) external payable{ } ``` - Thus, Using ``msg.value`` inside a loop is dangerous because this might allow the sender to ``re-use`` the ``msg.value``. +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). From 692cf8a74c66cb5c63d6e8756c25bb17fa9f281f Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Mon, 10 Jun 2024 22:42:38 +0545 Subject: [PATCH 06/11] Lack of precision vuln updated to precision loss --- README.md | 2 +- vulnerabilities/lack-of-precision.md | 14 ---------- vulnerabilities/precision-loss.md | 39 ++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 15 deletions(-) delete mode 100644 vulnerabilities/lack-of-precision.md create mode 100644 vulnerabilities/precision-loss.md diff --git a/README.md b/README.md index b814488..21a432c 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,6 @@ - [Default Visibility](./vulnerabilities/default-visibility.md) - [Insufficient Access Control](./vulnerabilities/insufficient-access-control.md) - [Off-By-One](./vulnerabilities/off-by-one.md) -- [Lack of Precision](./vulnerabilities/lack-of-precision.md) +- [Precision Loss](./vulnerabilities/precision-loss.md) - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) - [Using ``msg.value`` in a Loop](./vulnerabilities/msgvalue-loop.md) diff --git a/vulnerabilities/lack-of-precision.md b/vulnerabilities/lack-of-precision.md deleted file mode 100644 index ef40b0d..0000000 --- a/vulnerabilities/lack-of-precision.md +++ /dev/null @@ -1,14 +0,0 @@ -## Lack of Precision - -In Solidity, there are a limited variety of number types. Differently from many programming languages, floating point numbers are unsupported. Fixed point numbers are partially supported, but cannot be assigned to or from. The primary number type in Solidity are integers, of which resulting values of calculations are always rounded down. - -Since division often results in a remainder, performing division with integers generally requires a lack of precision to some degree. To see how a lack of precision may cause a serious flaw, consider the following example in which we charge a fee for early withdrawals denominated in the number of days early that the withdrawal is made: - -``` -uint256 daysEarly = withdrawalsOpenTimestamp - block.timestamp / 1 days -uint256 fee = amount / daysEarly * dailyFee -``` - -The problem with this is that in the case that a user withdraws 1.99 days early, since 1.99 will round down to 1, the user only pays about half the intended fee. - -In general, we should ensure that numerators are sufficiently larger than denominators to avoid precision errors. A common solution to this problem is to use fixed point logic, i.e. raising integers to a sufficient number of decimals such that the lack of precision has minimal effect on the contract logic. A good rule of thumb is to raise numbers to 1e18 (commonly referred to as WAD). \ No newline at end of file diff --git a/vulnerabilities/precision-loss.md b/vulnerabilities/precision-loss.md new file mode 100644 index 0000000..d24bf0c --- /dev/null +++ b/vulnerabilities/precision-loss.md @@ -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 * 2 / 10 = 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. \ No newline at end of file From 5816d4ffe8d172506d7a6c60105f33ed526e8f44 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Tue, 11 Jun 2024 08:11:45 +0545 Subject: [PATCH 07/11] precision loss vuln updated: added requested changes --- vulnerabilities/precision-loss.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vulnerabilities/precision-loss.md b/vulnerabilities/precision-loss.md index d24bf0c..7b57744 100644 --- a/vulnerabilities/precision-loss.md +++ b/vulnerabilities/precision-loss.md @@ -22,7 +22,7 @@ In the above code example, ``funcA()`` returns: ``` and ``funcB()`` returns: ```solidity -11 * 2 / 10 = 55 +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. From ca935da669c544f5f2a4b2e8316ba58806d90c6f Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Wed, 19 Jun 2024 12:59:29 +0545 Subject: [PATCH 08/11] merging precison loss --- vulnerabilities/precision-loss.md | 1 + 1 file changed, 1 insertion(+) diff --git a/vulnerabilities/precision-loss.md b/vulnerabilities/precision-loss.md index 7b57744..c46b5e6 100644 --- a/vulnerabilities/precision-loss.md +++ b/vulnerabilities/precision-loss.md @@ -36,4 +36,5 @@ 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. \ No newline at end of file From e39411815717a81907cd5f15df5216c5afe1521d Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Mon, 10 Jun 2024 22:42:38 +0545 Subject: [PATCH 09/11] Lack of precision vuln updated to precision loss --- vulnerabilities/precision-loss.md | 1 - 1 file changed, 1 deletion(-) diff --git a/vulnerabilities/precision-loss.md b/vulnerabilities/precision-loss.md index c46b5e6..7b57744 100644 --- a/vulnerabilities/precision-loss.md +++ b/vulnerabilities/precision-loss.md @@ -36,5 +36,4 @@ 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. \ No newline at end of file From 06a6c42c9d0060dce25fa11bcfeab21ebc11da14 Mon Sep 17 00:00:00 2001 From: 0xSandyy <0xSandyy@gmail.com> Date: Wed, 19 Jun 2024 13:51:21 +0545 Subject: [PATCH 10/11] rebased and dropped msgValue branch --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index ba845b5..771d42b 100644 --- a/README.md +++ b/README.md @@ -37,4 +37,5 @@ - [Precision Loss](./vulnerabilities/precision-loss.md) - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) - [Using ``msg.value`` in a Loop](./vulnerabilities/msgvalue-loop.md) +- [Using ``msg.value`` in a loop](./vulnerabilities/msgvalue-loop.md) - [Deleting a Mapping Within a Struct](./vulnerabilities/mapping-within-struct.md) From 66090112d416bb885a1b96378e6367b73ab2218f Mon Sep 17 00:00:00 2001 From: kaden Date: Mon, 10 Jun 2024 14:58:55 -0700 Subject: [PATCH 11/11] Update README.md --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 771d42b..ba845b5 100644 --- a/README.md +++ b/README.md @@ -37,5 +37,4 @@ - [Precision Loss](./vulnerabilities/precision-loss.md) - [Unbounded Return Data](./vulnerabilities/unbounded-return-data.md) - [Using ``msg.value`` in a Loop](./vulnerabilities/msgvalue-loop.md) -- [Using ``msg.value`` in a loop](./vulnerabilities/msgvalue-loop.md) - [Deleting a Mapping Within a Struct](./vulnerabilities/mapping-within-struct.md)