Native rounding support

[thedavidmeister]

Open Zeppelin seem to have a subset of the functions here, that only allow for integers rather than optimising for 18 decimal fixed point, but one thing they do have is a rounding direction

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/Math.sol#L136

    function mulDiv(uint256 x, uint256 y, uint256 denominator, Rounding rounding) internal pure returns (uint256) {
        uint256 result = mulDiv(x, y, denominator);
        if (rounding == Rounding.Up && mulmod(x, y, denominator) > 0) {
            result += 1;
        }
        return result;
    }

these rounding directions are important e.g. in ERC4626 that mandates rounding directions as part of the spec depending on whether shares are being minted/burned and how that is being calculated

generally it comes up whenever there's question of token dust that might cause some function to revert or some weird exploit to be possible if senders can get something for nothing

the workaround is to do exactly what OZ does with a wrapper but if it was "native" in the prb-math lib that would be a nicer package downstream

[PaulRBerg]

Thanks for posting this feature request, @thedavidmeister. I will move this issue to the discussions section because I keep issues strictly for settled todos and bug fixes.

PRBMath always rounds down, so issues of token dust should be ruled out. Still, I agree that rounding modes would be useful. It's just a question of finding the time to implement this; rounding modes would trigger quite a refactor in the tests.

[thedavidmeister]

@PaulRBerg the dust issue is not that simple, from ERC4626 https://eips.ethereum.org/EIPS/eip-4626

Finally, EIP-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.

If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

as i understand, the rounding math appeared in Open Zeppelin around the time they provided a 4626 vault implementation, it is part of the specification

the issue is that some of the methods on 4626 are framed like "how many shares do i need to provide to receive exactly X assets?" and if you round this down you allow the user to ask for a larger X assets for the same number of shares to burn, within the rounding error

the rounding behaviour specified by 4626 even leads to potential attacks OpenZeppelin/openzeppelin-contracts#3706 but it is what it is

maybe you don't call this "dust" but there are scenarios in major ERCs that require rounding other than unconditionally down

[thedavidmeister]

another place i've run into the desire to specify rounding up is when calculating prices/trades between counterparties, each side of the trade will naturally want to round up to give themselves the best possible deal

similarly prices from oracles, e.g. when creating synthetic prices by dividing price pairs from chainlink, perhaps we want to be rounding up when providing a price quote

[PaulRBerg]

Thanks a lot for the additional color, David. I think that I should assign a higher priority to this topic of rounding modes.

Am I understanding correctly that your feature request is particularly limited to the div and the mul functions, plus the mulDiv flavors in Common.sol?

Or were you interested in having the ability to specify the rounding mode in all PRBMath functions that contain divisions or multiplications in their internal implementations?

[thedavidmeister]

@PaulRBerg personally i've been pinged in audits by several auditing firms pretty much anywhere that loss of precision can happen and i haven't explicitly stated a rounding strategy, either to document it to raise awareness for users, or to implement some kind of rounding up if appropriate

so a standalone mul is probably not in scope by itself, but any time there's a div involved we can lose precision and so would have to specify it, so that would include mulDiv

[PaulRBerg]

Thanks a lot for your feedback.

[thedavidmeister]

also for more context, i've pulled your lib into the onchain interpreter that i'm working on rainlanguage/rain-protocol#605

ideally ppl who write expressions to be evaluated can specify the rounding direction themselves, which could be important in situations like tracking token amounts across several different tokens with different decimals where rescaling has to occur before decimal math can be performed, e.g. DAI is 18 decimals and USDT is 6 decimals

[PaulRBerg]

Makes complete sense.

[PaulRBerg]

Closing in favor of #186.