Some possible arithmetic issues/optimizations in the generated #utility.yul

[minhhn2910]

Description

When testing with migrating some simple contracts from solc-7 to solc-8, our in-house evm bytecode fuzzing tool starts to complain about arithmetic bugs in #utility.yul that is injected since solc-8. Although these alerts actually do not affect the output, we want to ask if these behaviors are all intended or they will be optimized in the future ?

Environment

• Compiler version: 0.8.15+commit.e14f2714 --optimize.
• Target EVM version : default
• Framework/IDE : N/A
• EVM execution environment / backend / blockchain client: py-evm

Steps to Reproduce

pragma solidity ^0.8.15;
contract example{
    uint8 a; uint8 b;
    function test() public{
        a++;
	b = b*a;
    }
}

---

Bug1: division by zero
#utility.yul 486:500
line 19:         if and(iszero(iszero(x_1)), gt(y_1, div(0xff, x_1))) { panic_error_0x11() }
arithmetic log: 0 = 255 / 0
----------------------------------------------------------------------------------
Potential bug2: Integer underflow 
#utility.yul 244:261
line 12:         if eq(value_1, 0xff) { panic_error_0x11() }
arithmetic log : 115792089237316195423570985008687907853269984665640564039457584007913129639681 == 0 - 255

---

Comments

Bug1 happens whenever a multiplication is used with x=0, the checked_mul_t_uint8(x, y) in yul code:

function checked_mul_t_uint8(x, y) -> product {
                x := cleanup_t_uint8(x)
                y := cleanup_t_uint8(y)
                // overflow, if x != 0 and y > (maxValue / x)
                if and(iszero(iszero(x)), gt(y, div(0xff, x))) { panic_error_0x11() }
                product := mul(x, y)
            }

If the x/0 = 0 is intended following Ethereum yellow paper, non-orthodox evm or web-assembly runtime implementations (where they throw exception with div/0) will revert tx if we do checked_multiply with 0

---

Potential Bug2 is not a bug and actually is the compiler generating the check for increment_t_uint8 for a++

function increment_t_uint8(value) -> ret {
                value := cleanup_t_uint8(value)
                if eq(value, 0xff) { panic_error_0x11() }
                ret := add(value, 1)
            }

The eq(value, 0xff) is implemented by subtraction which doesvalue - 0xff everytime the function is called with the trace

OPCODE: 0x60 (PUSH1) | pc: 166 | stack: [... '0x00', '0x45', '0x0'] 
OPCODE: 0x60 (PUSH1) | pc: 168 | stack: [... '0x45', '0x0', '0x00'] 
OPCODE: 0x82 (DUP3) | pc: 170 | stack: [..., '0x0', '0x00', '0xff'] 
OPCODE: 0x16 (AND) | pc: 171 | stack: [..., '0x0', '0x00', '0xff', '0x0'] 
OPCODE: 0x60 (PUSH1) | pc: 172 | stack: [..., '0x0', '0x00', '0x0'] 
OPCODE: 0x81 (DUP2) | pc: 174 | stack: [..., '0x00', '0x0', '0xff'] 
OPCODE: 0x3 (SUB) | pc: 175 | stack: [..., '0xff', '0x0'] 
OPCODE: 0x60 (PUSH1) | pc: 176 | stack: [..., '0x0', '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01'] 
OPCODE: 0x57 (JUMPI) | pc: 178 | stack: [..., '0x00', '0x0', '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff01', '0xb8'] 

Please correct me if I'm wrong, isn't it more gas efficient to do a GT check on 0xfe to replace eq(value, 0xff) instead of the above sequence ? It may save significant gas in the for loops using increment value.

[ekpyron]

The EVM defines division by zero to be zero, not as a revert (as you say yourself) - since our code generation targets an EVM following that specification, I'd not consider divisions by zero a bug - I don't think it's really practical for us to take any potential "non-orthodox" EVM implementation into account :-). That being said, for those particular multiplication overflow checks, we may change the code generation to a division-free version in the near future - at least in some cases, but even then I couldn't guarantee that the generated bytecode will never contain a division by zero, since given the EVM spec it's perfectly valid.

Regarding the second point: the reason why we need masking before the equality comparison is that the generated code is meant to be robust against dirty higher-order bits in value. Consider

function f() public {
  uint8 x;
  assembly { x := 0x0100 }
  x++;
}

Here x is treated as having an effective value of zero, even after the inline assembly write and therefore the increment does not overflow. If the overflow check was implemented using gt(x,0xfe), however, it would kick in and would revert.

It is an arguable design choice to prefer robustness against dirty values - but that's how things worked traditionally and changing that would be a breaking change...

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

[github-actions[bot]]

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.