fix: rounding in the protocol's favor #468

0xJabberwock · 2024-04-03T06:48:05Z

Description

After thoroughly reviewing the BasePoolMath library in search of rounding inconsistencies, I've found some cases in which multiplications (mulUp / mulDown) or divisions (divUp / divDown) weren't favorable to the protocol.

Type of change

Checklist:

The diff is legible and has no extraneous changes
Complex code has been commented, including external interfaces
Tests have 100% code coverage
The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #395.

openzeppelin-code · 2024-04-03T06:57:01Z

fix: rounding in the protocol's favour

Generated at commit: 2771ba7a09582fde5475b4eafaad31f6d7bbbd31

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 1 0 10 36 49
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

0xJabberwock · 2024-04-03T06:52:08Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

-        for (uint256 i = 0; i < balances.length; i++) {
+        for (uint256 i = 0; i < balances.length; ++i) {


Prefix increments and decrements (++i; --i) are cheaper than postfix increments and decrements (i++; i--) in terms of gas.

how come no gas snapshots have changed? 👀

i think there are a lot of loops around the codebase, let's try that in a new PR and see the gas snapshots improvements (avoid cross-function prs when possible)

Also, there's a 1% threshold on the gas snapshots, so maybe they did change, just less than 1%

I've made further research regarding the gas costs incurred when using prefix or postfix increments or decrements, specifically within for loop declarations. It seems that compiling via_ir = true equals the gas expenditures. For more details, refer to ethereum/solidity#14595.

0xJabberwock · 2024-04-03T07:05:27Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

-            if (newBalances[index] > invariantRatio.mulUp(currentBalances[index])) {
-                uint256 taxableAmount = newBalances[index] - invariantRatio.mulUp(currentBalances[index]);
+            if (newBalances[index] > invariantRatio.mulDown(currentBalances[index])) {
+                uint256 taxableAmount = newBalances[index] - invariantRatio.mulDown(currentBalances[index]);


The protocol is interested in taxing a larger amount in order to charge more swap fees which, in turn, help in reducing the amount of BPT to bid during addLiquidityUnbalanced.

0xJabberwock · 2024-04-03T07:20:14Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

-        // mulUp/divDown maximize the amount of tokens burned for the security reasons
-        bptAmountIn = totalSupply.mulUp(currentInvariant - invariantWithFeesApplied).divDown(currentInvariant);
+        // mulUp/divUp maximize the amount of tokens burned for the security reasons
+        bptAmountIn = totalSupply.mulUp(currentInvariant - invariantWithFeesApplied).divUp(currentInvariant);


The protocol is interested in maximizing the amount of BPT to ask for during removeLiquiditySingleTokenExactOut.

0xJabberwock · 2024-04-03T07:24:04Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

        // Calculate the non-taxable balance proportionate to the BPT burnt.
-        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divDown(totalSupply);
+        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divUp(totalSupply);

        // Compute the taxable amount: the difference between the non-taxable balance and actual withdrawal.
        uint256 taxableAmount = nonTaxableBalance - newBalance;


The protocol is interested in taxing a larger amount in order to charge more swap fees which, in turn, help in reducing the amount of tokenOut to bid during removeLiquiditySingleTokenExactIn.

but isn't this increasing the nonTaxable part?

Perhaps rename nonTaxableBalance, the tax is calculated from it, which implies the tax is actually a part of nonTaxableBalance.

balanceBeforeTax maybe?

Good points. balanceBeforeTax or newProportionalBalance could be better suited. I had the same question as @wei3erHase when we first put this code together.

Should check if there's anywhere else where "taxable" or "nonTaxable" is used confusingly. Should add a comment about the rounding direction after fixing the names.

0xJabberwock · 2024-04-03T07:24:47Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

        // Calculate the taxable amount, which is the difference
        // between the actual amount in and the non-taxable balance
-        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenInIndex]).divDown(totalSupply);
+        uint256 nonTaxableBalance = newSupply.mulDown(currentBalances[tokenInIndex]).divDown(totalSupply);

        uint256 taxableAmount = (amountIn + currentBalances[tokenInIndex]) - nonTaxableBalance;


The protocol is interested in taxing a larger amount in order to charge more swap fees which, in turn, help in augmenting the amount of tokenIn to ask for during addLiquiditySingleTokenExactOut.

wei3erHase · 2024-04-03T10:13:49Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            let cap := add(numTokens, 1)
+            for {
+                let i := 1
+            } lt(i, cap) {
+                i := add(i, 1)
+            } {
+                let pos := mul(i, 0x20)
+                mstore(add(newBalances, pos), mload(add(currentBalances, pos)))
+            }
        }
+


interesting, would you run ./pkg/vault$ yarn test:forge to see if there are gas savings with this?

I think this TODO is inherited from V2 code. I'd leave it out of this PR; it doesn't sound very important.

Sidenote: there's now an mcopy opcode since the last hardfork.
There are other places where we're copying arrays, although it's usually for scaling (we have to do math on every element anyways, so perhaps mcopy doesn't save much). See #308.

In V3 we're avoiding assembly if at all possible (e.g., not with transients quite yet), generally favoring clarity/ease of review/audit over speed.

interesting, would you run ./pkg/vault$ yarn test:forge to see if there are gas savings with this?

Indeed, there are some gas savings as a result of both gas optimizations introduced. However, I agree that they should be addressed separately in a further PR so as to target single concerns.

jubeira

Looks good, thanks @0xJabberwock!

As @wei3erHase points out, let's please try to keep PRs targeting single concerns whenever possible.
For this one in particular, the pre-increments in the for loops are fine, but let's leave the assembly optimization for another one. The rest looks good!

jubeira · 2024-04-03T12:05:55Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            let cap := add(numTokens, 1)
+            for {
+                let i := 1
+            } lt(i, cap) {
+                i := add(i, 1)
+            } {
+                let pos := mul(i, 0x20)
+                mstore(add(newBalances, pos), mload(add(currentBalances, pos)))
+            }
        }
+


I think this TODO is inherited from V2 code. I'd leave it out of this PR; it doesn't sound very important.

Sidenote: there's now an mcopy opcode since the last hardfork.
There are other places where we're copying arrays, although it's usually for scaling (we have to do math on every element anyways, so perhaps mcopy doesn't save much). See #308.

jubeira · 2024-04-03T12:08:14Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

        // Calculate the non-taxable balance proportionate to the BPT burnt.
-        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divDown(totalSupply);
+        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divUp(totalSupply);

        // Compute the taxable amount: the difference between the non-taxable balance and actual withdrawal.
        uint256 taxableAmount = nonTaxableBalance - newBalance;


Good points. balanceBeforeTax or newProportionalBalance could be better suited. I had the same question as @wei3erHase when we first put this code together.

jubeira · 2024-04-03T12:32:23Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

@@ -265,8 +274,8 @@ library BasePoolMath {
        swapFeeAmounts = new uint256[](numTokens);
        swapFeeAmounts[tokenOutIndex] = fee;

-        // mulUp/divDown maximize the amount of tokens burned for the security reasons
-        bptAmountIn = totalSupply.mulUp(currentInvariant - invariantWithFeesApplied).divDown(currentInvariant);
+        // mulUp/divUp maximize the amount of tokens burned for the security reasons


Suggested change

// mulUp/divUp maximize the amount of tokens burned for the security reasons

// mulUp/divUp maximizes the amount of tokens burned for security reasons

Do we just want to say "to favor the protocol," as elsewhere, instead of "for security reasons"? We are favoring the protocol for security reasons, but maybe we can say that in one place at the top, e.g.: "For security reasons, to help ensure that for all possible "round trip" paths, the caller always receives the same or fewer tokens than supplied, we have chosen the rounding direction to favor the protocol in all cases."

EndymionJkb · 2024-04-03T14:30:56Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

@@ -159,7 +159,7 @@ library BasePoolMath {

        // Calculate the amount of BPT to mint. This is done by multiplying the
        // total supply with the ratio of the change in invariant.
-        // mulDown/divDown minize amount of pool tokens to mint.
+        // mulDown/divDown minimize amount of pool tokens to mint.


Suggested change

// mulDown/divDown minimize amount of pool tokens to mint.

// mulDown/divDown minimizes the amount of pool tokens to mint.

For consistency with other comments. It seems like it should be singular if you read it "mulDown and divDown", but you can think of it as "the combination of mulDown/divDown," or the "mulDown/divDown expression" which would make it minimizes.

EndymionJkb · 2024-04-03T14:35:57Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

@@ -141,11 +141,11 @@ library BasePoolMath {
        uint256 invariantRatio = newInvariant.divDown(currentInvariant);

        // Loop through each token to apply fees if necessary.
-        for (uint256 index = 0; index < currentBalances.length; index++) {
+        for (uint256 index = 0; index < numTokens; ++index) {
            // Check if the new balance is greater than the proportional balance.
            // If so, calculate the taxable amount.


Suggested change

// If so, calculate the taxable amount.

// If so, calculate the taxable amount, rounding in favor of the protocol.

// We round the invariantRatio term down to subtract less and get a higher `taxableAmount`, which

// charges higher swap fees, reducing the amount of BPT that will be minted.

As elsewhere, I'd comment the reasoning for the rounding direction, so that it can be explicitly reviewed, and we're not always reverse engineering it.

EndymionJkb · 2024-04-03T14:39:14Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

@@ -191,13 +191,13 @@ library BasePoolMath {
        uint256 newSupply = exactBptAmountOut + totalSupply;
        // Calculate the initial amount of the input token needed for the desired amount of BPT out
        // "divUp" leads to a higher "newBalance," which in turn results in a larger "amountIn."
-        // This leads to receiving more tokens for the same amount of BTP minted.
+        // This leads to receiving more tokens for the same amount of BPT minted.
        uint256 newBalance = computeBalance(currentBalances, tokenInIndex, newSupply.divUp(totalSupply));
        uint256 amountIn = newBalance - currentBalances[tokenInIndex];

        // Calculate the taxable amount, which is the difference
        // between the actual amount in and the non-taxable balance


Suggested change

// between the actual amount in and the non-taxable balance

// between the actual amount in and the non-taxable balance. Round the `nonTaxableBalance` down to

// favor the protocol by increasing the taxable amount, which charges higher swap fees, ultimately increasing

// the amount of `tokenIn` that will be transferred from the caller.

EndymionJkb · 2024-04-03T14:41:26Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            let cap := add(numTokens, 1)
+            for {
+                let i := 1
+            } lt(i, cap) {
+                i := add(i, 1)
+            } {
+                let pos := mul(i, 0x20)
+                mstore(add(newBalances, pos), mload(add(currentBalances, pos)))
+            }
        }
+


In V3 we're avoiding assembly if at all possible (e.g., not with transients quite yet), generally favoring clarity/ease of review/audit over speed.

EndymionJkb · 2024-04-03T14:48:56Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

@@ -265,8 +274,8 @@ library BasePoolMath {
        swapFeeAmounts = new uint256[](numTokens);
        swapFeeAmounts[tokenOutIndex] = fee;

-        // mulUp/divDown maximize the amount of tokens burned for the security reasons
-        bptAmountIn = totalSupply.mulUp(currentInvariant - invariantWithFeesApplied).divDown(currentInvariant);
+        // mulUp/divUp maximize the amount of tokens burned for the security reasons


Do we just want to say "to favor the protocol," as elsewhere, instead of "for security reasons"? We are favoring the protocol for security reasons, but maybe we can say that in one place at the top, e.g.: "For security reasons, to help ensure that for all possible "round trip" paths, the caller always receives the same or fewer tokens than supplied, we have chosen the rounding direction to favor the protocol in all cases."

EndymionJkb · 2024-04-03T14:50:44Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

        // Calculate the non-taxable balance proportionate to the BPT burnt.
-        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divDown(totalSupply);
+        uint256 nonTaxableBalance = newSupply.mulUp(currentBalances[tokenOutIndex]).divUp(totalSupply);

        // Compute the taxable amount: the difference between the non-taxable balance and actual withdrawal.
        uint256 taxableAmount = nonTaxableBalance - newBalance;


Should check if there's anywhere else where "taxable" or "nonTaxable" is used confusingly. Should add a comment about the rounding direction after fixing the names.

0xJabberwock · 2024-04-04T08:33:01Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

+        // Compute the taxable amount: the difference between the new proportional and disproportional balances.
+        uint256 taxableAmount = newBalanceBeforeTax - newBalance;


During computeRemoveLiquiditySingleTokenExactIn, it turns out that, in order for the taxableAmount to be as large as possible, we would want newBalanceBeforeTax to be rounded up and newBalance to be rounded down.

Nevertheless, newBalance was previously subtracted from currentBalances[tokenOutIndex] so as to compute amountOut; since we'd want the latter to be as low as possible, newBalance had been rounded up initially.

In consequence, a conflict of interests arises regarding the rounding of newBalance.

I think it's fine.
Taxable balance is not as important as amounts out to withdraw.

Might be good to document this rounding conflict and priority in rounding amountOut over taxableAmount so we need not repeat thinking through this again :)

Yes, we should always document the reason for the rounding: see #468 (comment)

jubeira

This is looking good IMO, thanks @0xJabberwock !
@EndymionJkb @joaobrunoah would you mind giving it one last look before merging?

/cc @HickupHH3

jubeira · 2024-04-04T13:08:12Z

pkg/solidity-utils/contracts/math/BasePoolMath.sol

+        // Compute the taxable amount: the difference between the new proportional and disproportional balances.
+        uint256 taxableAmount = newBalanceBeforeTax - newBalance;


I think it's fine.
Taxable balance is not as important as amounts out to withdraw.

EndymionJkb

Looks great. The only thing we might still want to address is the comment in computeRemoveLiquiditySingleTokenExactIn:

"In consequence, a conflict of interests arises regarding the rounding of newBalance."

0xJabberwock added 2 commits April 3, 2024 03:23

fix: rounding in the protocol's favour

c36855d

refactor: copy memory array in Yul

9bfa105

0xJabberwock self-assigned this Apr 3, 2024

0xJabberwock commented Apr 3, 2024

View reviewed changes

refactor: lint Yul

2771ba7

0xJabberwock requested review from jubeira, wei3erHase, HickupHH3, EndymionJkb and ylv-io April 3, 2024 07:52

wei3erHase reviewed Apr 3, 2024

View reviewed changes

jubeira reviewed Apr 3, 2024

View reviewed changes

EndymionJkb reviewed Apr 3, 2024

View reviewed changes

0xJabberwock added 4 commits April 4, 2024 04:40

refactor: clearer variable names and comments

78927e4

refactor: revert non-trivial gas optimizations

8cc432e

chore: merge branch 'main'

74d50f8

refactor: delete assembly TODO

75ed2fd

0xJabberwock commented Apr 4, 2024

View reviewed changes

jubeira approved these changes Apr 4, 2024

View reviewed changes

EndymionJkb approved these changes Apr 4, 2024

View reviewed changes

0xJabberwock added 2 commits April 4, 2024 17:31

refactor: comment rounding conflict of interests

09d7635

chore: merge branch 'main'

bca61a8

0xJabberwock changed the title ~~fix: rounding in the protocol's favour~~ fix: rounding in the protocol's favor Apr 4, 2024

0xJabberwock merged commit 1e3e74d into main Apr 4, 2024
10 checks passed

0xJabberwock deleted the fix/base-pool-math-rounding branch April 4, 2024 20:50

This was referenced Apr 4, 2024

refactor: normalize for loop declarations #476

Merged

Consider using mcopy for scaling functions #308

Closed

fix: rounding in the protocol's favor pt. 2 #487

Merged

fix: rounding in the protocol's favor pt. 3 #489

Merged

joaobrunoah mentioned this pull request Apr 16, 2024

Test rounding scenarios in Base Math, Weighted Math and Stable Math #505

Closed

		for (uint256 i = 0; i < balances.length; i++) {
		for (uint256 i = 0; i < balances.length; ++i) {

	// mulUp/divUp maximize the amount of tokens burned for the security reasons
	// mulUp/divUp maximizes the amount of tokens burned for security reasons

	// mulDown/divDown minimize amount of pool tokens to mint.
	// mulDown/divDown minimizes the amount of pool tokens to mint.

-            // If so, calculate the taxable amount.
+            // If so, calculate the taxable amount, rounding in favor of the protocol.
+            // We round the invariantRatio term down to subtract less and get a higher `taxableAmount`, which
+            // charges higher swap fees, reducing the amount of BPT that will be minted.

-        // between the actual amount in and the non-taxable balance
+        // between the actual amount in and the non-taxable balance. Round the `nonTaxableBalance` down to
+        // favor the protocol by increasing the taxable amount, which charges higher swap fees, ultimately increasing
+        // the amount of `tokenIn` that will be transferred from the caller.

		// Compute the taxable amount: the difference between the new proportional and disproportional balances.
		uint256 taxableAmount = newBalanceBeforeTax - newBalance;

fix: rounding in the protocol's favor #468

fix: rounding in the protocol's favor #468

Conversation

0xJabberwock commented Apr 3, 2024

Description

Type of change

Checklist:

Issue Resolution

openzeppelin-code bot commented Apr 3, 2024 • edited Loading

fix: rounding in the protocol's favour

🚨 Report Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wei3erHase Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira Apr 3, 2024 • edited by EndymionJkb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubeira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EndymionJkb left a comment

Choose a reason for hiding this comment

openzeppelin-code bot commented Apr 3, 2024 •

edited

Loading

wei3erHase Apr 3, 2024 •

edited

Loading

jubeira Apr 3, 2024 •

edited by EndymionJkb

Loading