Update overflow-underflow.md

[indeqs]

Related Issue

• Closes Updated overflow-underflow.md with more details #81

Checklist

• I have read and followed the style guide

Describe the changes you've made:

Updated overflow-underflow.md with more examples on using YUL

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)

[kadenzipfel]

In most circumstances, we'd rather have execution revert rather than actually overflowing the output. In fact, I can't really think of any circumstances where we wouldn't

[kadenzipfel]

Yeah I don't think we should suggest allowing the overflow

[kadenzipfel]

I kinda feel like this section just leads to confusion because it's really quite similar to what would happen when not dropping down to assembly

[kadenzipfel]

Well actually, my initial assumption was that it reverts when we try to return outputTokens as a uint128, but perhaps this isn't the case? If so this would be good to clarify

[indeqs]

I kinda feel like this section just leads to confusion because it's really quite similar to what would happen when not dropping down to assembly

which section specifically are you talking about?

[indeqs]

Well actually, my initial assumption was that it reverts when we try to return outputTokens as a uint128, but perhaps this isn't the case? If so this would be good to clarify

about this @kadenzipfel, it took me a while to wrap my head around it, but here is what I found out:-

---

1. Function Definition:

   function getSwapQuoteUint128(uint128 amountToSwap) external view returns (uint128 outputTokens) {
       assembly {
           outputTokens := add(amountToSwap, 1)
           if lt(outputTokens, amountToSwap) { revert(0, 0) }
       }
   }

   This function is intended to return amountToSwap + 1 and detect an overflow if outputTokens is less than amountToSwap.

2. 256-bit Arithmetic:
   In the EVM, arithmetic operations within inline assembly (Yul) are performed using 256-bit values. When adding two 128-bit values, the result is treated as a 256-bit value.

3. Overflow Check:
   The function includes an overflow detection mechanism:

   if lt(outputTokens, amountToSwap) { revert(0, 0) }

   This checks if outputTokens is less than amountToSwap.

4. Overflow Scenario:
   When amountToSwap is type(uint128).max (2^128 - 1), adding 1 results in 2^128, which fits in a 256-bit register but exceeds the 128-bit range. The result is a valid 256-bit value (2^128), but it cannot be represented as a 128-bit value.

5. Implicit Conversion:
   When the 256-bit result is assigned to a 128-bit variable (outputTokens), it truncates the higher bits, resulting in 0 (since 2^128 overflows a 128-bit integer back to 0).

6. Assertion:
   The assertion confirms that the function indeed returns 0 when given the maximum uint128 value due to the overflow:

   assertEq(0, dexPair.getSwapQuoteUint128(type(uint128).max));

The key issue here is that the overflow detection mechanism:

if lt(outputTokens, amountToSwap) { revert(0, 0) }

fails because outputTokens and amountToSwap are treated as 256-bit values during the addition operation. When outputTokens is 2^128, it is not less than amountToSwap in 256-bit arithmetic. Therefore, the check lt(outputTokens, amountToSwap) does not trigger a revert, even though the actual result overflows when considering 128-bit constraints.

TLDR;

The inline assembly overflow check fails because the EVM uses 256-bit arithmetic, while the function is supposed to handle 128-bit values. This discrepancy allows an overflow to evade detection, causing the function to return incorrect results without reverting. Specifically, adding 1 to the maximum uint128 value (2^128 - 1) results in an overflow to 0 instead of triggering the overflow detection.

[kadenzipfel]

Yeah I think this section is has too much overlap with the above section for it to be necessary

[kadenzipfel]

I'm not clear on what this note is indicating

[indeqs]

It was supposed to indicate the pitfall a dev may fall into when implementing an overflow check in assembly using the patterns discussed.

Perhaps it wasn't clear :)

[kadenzipfel]

Yeah I think this section is has too much overlap with the above section for it to be necessary

[kadenzipfel]

Is this not the same as number 2?

[kadenzipfel]

We can probably simply the mitigation section to just:

• Revert if an over/underflow will occur (will happen regardless with safemath/v>0.8)
• Don't use unchecked blocks unless over/underflow is impossible

[kadenzipfel]

Yeah I don't think we should suggest allowing the overflow