GeneralIndexModule internal review and external audit revisions

[cgewecke]

This is a set of small revisions suggested by the internal review of GeneralIndexModule. Each idea is expressed as a commit, so it can be considered on it's own and cherry-picked if useful.

The existing contract is highly optimized for gas - it minimizes state reads in loops as well as method encapsulation that would impose costs for extra function calls.

This version of the contract does the opposite - it introduces additional getters and generally ignores state read costs. For example, _normalizedTargetUnits is removed. It's also more aggressive about param validation. checks are added for

• the public getters: these might be used in automated scripts and ambiguous outputs are generated for invalid inputs
• the exchange name is validated in setExchange (it's also validated during trades). Also storing this as a bytes32 hash, mirroring the IntegrationRegistry.

The Berlin hardfork has made initial state reads for a location more expensive, but subsequent "warm" reads from the same place are much cheaper, changing the gas calculus a bit. Despite the inefficiencies introduced in this draft, the diff between versions suggests that gas costs increased only marginally overall. They would increase more in a module with a larger number of components.

---

Update I

Additional commits have been added after the review was finalized on Monday. Notable things are:

• storage of exchangeNames as bytes32 hashes has been reverted following initial review here - these are strings again
• initialize and startRebalance now revert if there are external positions for a component
• _createTradeInfo and _createTradeRemainingInfo have been substantially restructured
  • common logic has been pulled out and encapsulated in additional helpers
  • the field naming for the TradeInfo struct has changed to more closely match TradeModule's syntax:
    • from "fixed/floatingQuantity" to sendQuantity and minReceiveQuantity
    • from isSendFixed to isSell

---

Update II: (Internal review remainders, review feedback, audit corrections)

TODO:

• Revert isSell, minReceivedQuantity to isFixedSend, floatingQuantityLimit
• Move _validate array methods to AddressArrayLibs (and add additional)
• Optimize external position detection
• Optimize modifier inlining
• Declare TradeInfo as return parameter
• Add Position.unitsMatchWithinRange method for targets logic (invert)
• Restrict _getAggregateComponentsAndUnits state mutability to pure
• Iosoro 5.3.2: add removeModule tests for preserved state, eval trader deletion
• Iosoro 5.4.1: event (w/ position multiplier) for raiseAssetTarget
• Iosoro 5.4.3: immutable weth
• Iosoco 5.4.3: typos
• Ask Felix for clarification about fees ambiguity in _calculateTradeSizeAndDirection
  • Response:

  • It’s a code organization comment, where either 1) all fee-related calculations and logic should be grouped together or 2) a big comment is used to flag that logic and how it interacts w/ the rest of the module. In addition, I wanted to ascertain that there are no problematic edge cases and that the math is correct

• Update gas analysis
• Re-review docs and normalize namings
• Address fee concern on _calculateTradeSizeAndDirection
  • move fn level comment about issue inline
  • check test cases
• fix validatePairs bytes array method
• rename PositionMultiplier update --> AssetTargetsRaised
• remove de-authorized traders from history
• add changelog entry in ... (one of the libs)

Report shows original GeneralIndexModule vs. this PR (run on [email protected])

(Updated for most recent commits - April 20)

Deployments

	Gas		Diff	Diff %	Block %	usd avg
GeneralIndexModule	4,358,869		-45,230	-1%	35 %	1389.93

Methods

	Gas		Diff	Diff %	Calls	usd avg
GeneralIndexModule
initialize	189,082		-35,895	-16%	12	60.29
raiseAssetTargets	82,425		+1,159	+1%	6	26.28
setAnyoneTrade	62,487		-	-	4	19.93
setCoolOffPeriods	121,277		+7,130	+6%	4	38.67
setExchanges	120,933		+31,341	+35%	6	38.56
setRaiseTargetPercentage	62,616		-	-	12	19.97
setTradeMaximums	93,017		+8,397	+10%	7	29.66
setTraderStatus	109,688		-	-	13	34.98
startRebalance	193,078		+23,031	+14%	30	61.57
trade	364,066		-1,932	-1%	59	116.09
tradeRemainingWETH	286,393		-226	0%	9	91.32

Build Configuration

Option	Settings
solc: version	0.6.10
solc: optimized	true
solc: runs	200
gas: block limit	12,450,000
gas: price	123 gwei/gas
gas: currency/eth rate	2592.47 usd/eth

[bweick]

What are the benefits of storing the exchange name as a hash? To me this hurts readability.

[bweick]

So with Berlin would we expect the gas increase to be slightly less? (I'm assuming this wasn't run with Berlin changes)

[cgewecke]

What are the benefits of storing the exchange name as a hash? To me this hurts readability.

Agree, it's not as nice. There's a note in the review about the cost of storing them as a string.

Note that exchangeNames as strings will be very expensive to store and update

[cgewecke]

@bweick Yes, the gas report was run on a Berlin EVM. In Istanbul I'd expect these changes to increase costs a lot, but with Berlin they're more expensive but...by a smaller margin.

[bweick]

What are the benefits of storing the exchange name as a hash? To me this hurts readability.

Agree, it's not as nice. There's a note in the review about the cost of storing them as a string.

Note that exchangeNames as strings will be very expensive to store and update

Gotcha, that makes sense, though am I reading the table right that the cost of setting exchanges went up?

[cgewecke]

@bweick Yes, it went up quite a bit, but it's also validating the exchange name now. From the internal review for setExchange....

Validate that the exchangeName is on the integrationRegistry

This is a redundant check b/c the trade methods also check. But maybe makes sense from a UX perspective? In practice how frequently are the settings changed?

[bweick]

@bweick Yes, it went up quite a bit, but it's also validating the exchange name now. From the internal review....

Validate that the exchangeName is on the integrationRegistry

This is a redundant check b/c the trade methods also check. But maybe makes sense from a UX perspective? In practice how frequently are the settings changed?

I like the check here, if we could remove the check on the trade that'd be great since that is redundant but I think our inherited helper functions there enforce a check?

Maybe we can discuss a bit as a group but the readability is really nice. Do we know what the marginal cost of storing a string vs bytes32 is? I know it's going to be different based on string length but would be great to isolate the "cost of readability".

[cgewecke]

@bweick

Changing back to string increases the costs for setExchanges 14% (from this PR as a baseline, with the tests using 4 components). The deployment cost increased 3%.

With a single exchange name longer than 32bytes setExchanges increased 24%. (Ex: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)

There was no impact on the trade methods.

In the end this is just a few dollars for sets with a small number of components...if exchanges aren't configured very often idk that these gas differences matter that much.

[cgewecke]

if we could remove the check on the trade that'd be great

Do you know if this is possible? I was thinking that validating the adapter is necessary in trade because it could have been removed at some point between setting the exchanges and calling trade.

[bweick]

if we could remove the check on the trade that'd be great

Do you know if this is possible? I was thinking that validating the adapter is necessary in trade because it could have been removed at some point between setting the exchanges and calling trade.

Well the only way it could be removed is through setExchanges, in which case a valid exchange would need to be passed in its place. Unless there's a path to removal I'm not thinking of? Also usually we update maybe an exchange or two each time. Obviously if we're adding new components that also requires setting an exchange.

[cgewecke]

Can the adapter be removed via the IntegrationRegistry contract? I wrote a test for this possibility in #trade but maybe it's not right...

set-protocol-v2/test/protocol/modules/generalIndexModule.spec.ts

Lines 910 to 927 in 6f96431

	describe("when exchange adapter has been removed from integration registry", async () => {
	beforeEach(async () => {
	await indexModule.setExchanges(subjectSetToken.address, [subjectComponent], [balancerAdapterName]);
	await setup.integrationRegistry.removeIntegration(indexModule.address, balancerAdapterName);
	});
	afterEach(async () => {
	await setup.integrationRegistry.addIntegration(
	indexModule.address,
	balancerAdapterName,
	balancerExchangeAdapter.address
	);
	});
	it("the trade reverts", async () => {
	await expect(subject()).to.be.revertedWith("Must be valid adapter");
	});
	});

[bweick]

Can the adapter be removed via the IntegrationRegistry contract? I wrote a test for this possibility in #trade but maybe it's not right...

set-protocol-v2/test/protocol/modules/generalIndexModule.spec.ts

Lines 910 to 927 in 6f96431

	describe("when exchange adapter has been removed from integration registry", async () => {
	beforeEach(async () => {
	await indexModule.setExchanges(subjectSetToken.address, [subjectComponent], [balancerAdapterName]);
	await setup.integrationRegistry.removeIntegration(indexModule.address, balancerAdapterName);
	});
	afterEach(async () => {
	await setup.integrationRegistry.addIntegration(
	indexModule.address,
	balancerAdapterName,
	balancerExchangeAdapter.address
	);
	});
	it("the trade reverts", async () => {
	await expect(subject()).to.be.revertedWith("Must be valid adapter");
	});
	});

Ah yes it could that's a good catch. Then yes I'd say we should check it, even though it would revert if the adapter was invalid it'd be good to return a message.

[cgewecke]

This change from calldata to memory (in four methods) is due to migrating the array pair validators to AddressArrayUtils. There are compiler errors for both permutations of calldata/memory

• preserving calldata in this file and making the method signatures at lib and mocks calldata results in solc erroring with ... (this is a solc bug, according to the error type)

Using solcjs
Compiling 118 files with 0.6.10
InternalCompilerError: Invalid conversion to calldata type.

Error HH600: Compilation failed

• Keeping calldata signatures here and memory signatures there has solc error with:

Using solcjs
Compiling 118 files with 0.6.10
UnimplementedFeatureError: Copying nested dynamic calldata arrays to memory is not implemented in the old code generator.

Error HH600: Compilation failed

[cgewecke]

Note: this returns true if vals are equal up to and including +/- range (preserving the logic at _targetUnmet)

[bweick]

We have a method in AddressArrayUtils that's way cheaper called removeStorage, better than having to read it into memory, manipulate, and store.

[cgewecke]

d1ebe6e

[bweick]

Probably worth checking that the trader history array is updated properly as well

[cgewecke]

The tradersHistory is not available on the automatic getter generated by solidity. This is the permissionInfo abi.

{
  "inputs": [
    {
      "internalType": "contract ISetToken",
      "name": "",
      "type": "address"
    }
  ],
  "name": "permissionInfo",
  "outputs": [
    {
      "internalType": "bool",
      "name": "anyoneTrade",
      "type": "bool"
    }
  ],
  "stateMutability": "view",
  "type": "function",
  "gas": "0x81b320"
},

[cgewecke]

Only thing we can read out of that without writing a getter is anyoneTrade. Should we write one?

[cgewecke]

770b584