ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.11k stars 5.73k forks source link

Division by zero is checked in Unchecked blocks #15200

Closed Lohann closed 3 months ago

Lohann commented 3 months ago

Description

The division reverts even inside an unchecked block, which is contradictory, what if I already validated that before the unchecked block? or if I rely on the result zero when divide by zero?

Environment

Steps to Reproduce

The following code reverts when b is zero, which should not happen inside an unchecked block.

pragma solidity >=0.8.0;

contract Example {
    function divide(uint256 a, uint256 b) external pure returns (uint256) {
        unchecked {
            // Reverts when b = 0
            return a / b;
        }
    }

    function asmDivide(uint256 a, uint256 b) external pure returns (uint256 r) {
        assembly {
            // Works when b = 0
            r := div(a, b)
        }
    }
}
ompatil1357 commented 3 months ago

as per my observation , the unchecked blocks underflow checks and also the disable overflow . if we want to solve this bug you can just simply add the "check" before doing division calculation.

nikola-matic commented 3 months ago

Hi @Lohann. This isn't a bug, and is in fact by design, and has also been in Solidity since way before the unchecked arithmetic was implemented and became default in 0.8.0.

Division (and modulo) by zero wasn't actually reverting pre 0.4.21, and was reported in https://github.com/ethereum/solidity/issues/2694. It was then implemented in https://github.com/ethereum/solidity/pull/3523, and finally released as part of the 0.4.21 version.

It's also stated in the docs.

The reason for this is that division by zero abstract, and most languages either have it as undefined behaviour, or they'll throw an exception, or they may have an infinite value defined (e.g. Inf) which can be returned in such cases. In Solidity, the only thing that really makes sense is to revert, which is why it's the way it is, and why it won't change in the future.

If you're really adamant about not having this check (which in essence is just an ISZERO opcode with a conditional JUMPI), then you can implement the arithmetic in an inline assembly block. I would however advise against it, since I doubt that the gas gains would be significant enough to justify the 'risk'.

An important thing to note that may or may not be helpful is that the division by zero on the EVM level behaves by returning zero, i.e. x / 0 = 0, which from an arithmetic perspective, is not ideal as it can produce ambiguity.

Lohann commented 3 months ago

@nikola-matic I see, yeah I rely on the zero result in my algorithm, this is the saturating multiplication that consumes 46 gas:

function saturatingMul(uint256 a, uint256 b) external pure returns (uint256) {
    unchecked {
        uint256 c = a * b;
        bool success = (c / a) == b || a == 0; // Doesn't works because reverts here when a == 0
        uint256 limit = SafeCast.toUint(success) - 1;
        return c | limit;
    }
}

Assembly version:

PUSH1 0xBB // b
PUSH1 0xAA // a b

           // -- c = a * b
DUP2       // b a b
DUP2       // a b a b
MUL        // c a b

           // -- success = (c / a) == b || a == 0
SWAP2      // b a c
DUP2       // a b a c
DUP4       // c a b a c
DIV        // (c / a) b a c
EQ         // ((c / a) == b) a c
PUSH1 1    // 1 ((c / a) == b) a c
SWAP2      // a ((c / a) == b) 1 c
ISZERO     // (a == 0) ((c / a) == b) 1 c
OR         // success 1 c

           // -- limit = successs - 1
SUB        // limit c

           // -- result = success ? c : limit
OR         // result
nikola-matic commented 3 months ago

@Lohann in that case, and seeing as division by zero stopped yielding a zero since 0.4.21, your only option is to implement the desired behaviour inside an assembly block.