diff --git a/README.md b/README.md index 701ca18..ba845b5 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,7 @@ - [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) - [Deleting a Mapping Within a Struct](./vulnerabilities/mapping-within-struct.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/msgvalue-loop.md b/vulnerabilities/msgvalue-loop.md new file mode 100644 index 0000000..55105b7 --- /dev/null +++ b/vulnerabilities/msgvalue-loop.md @@ -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 \ No newline at end of file diff --git a/vulnerabilities/precision-loss.md b/vulnerabilities/precision-loss.md new file mode 100644 index 0000000..7b57744 --- /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 * 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. \ No newline at end of file