Spearbit post-audit

[clemlak]

Spearbit audit will end soon and the goal of this discussion is to plan what's next (code fixes, refactoring, etc...).

[clemlak]

Following the findings related to the encoding / decoding and the potential security risks that may be present, removing the custom calldata has been discussed. Here are some pros:

• Removing the custom calldata will simplify the codebase and reduce the size of the contract.
• It also reduces the room of potential errors due to calldata improperly encoded (using our own encoder or more specifically custom ones).
• Easier to interact with the protocol (from our front-end, or by 3rd parties).
• Calldata compression will be useless outside of optimistic rollups and will only increase the gas cost elsewhere.

[Alexangelj]

Also maybe doing the calldata slicing again? How do you feel about the decoders now that they were reviewed a bit

[clemlak]

Something that we mentioned in the past: https://github.com/spearbit-audits/review-primitive/issues/4.

Basically we need to add safety checks when it comes to adding / removing liquidity, we need to decide if we want to do it inside of the Portfolio contract or at a periphery level. Quick note: we might be running out of space for RMM01Portfolio, so removing the custom encoding might allow us to squeeze these checks.

[Alexangelj]

I'm considering removing "Collect", here's why:

• In RMM01Portfolio, it's only used in an edgecase that occurs when pools outperform and their invariant is positive.
• Fee reinvestment is a major feature that users love, when the edge case happens, collect removes that feature completely.
• It adds a significant amount of code and attack surface to the codebase.

[Alexangelj]

Great suggestion from infomorph:

I think in the FVM you could do a more thorough job pushing conversions to the edges, basically only at token transfer points.

[Alexangelj]

Meeting notes March 29, 2023

- First thing - we could remove custom abi encoding. Use regular functions.
	- Alex: It would mean we need a bytes[] array in multiprocess. Unless we do encodepacked in jump encoding. Individual instruction data = abi.encodeWithSelector, which has fixed word sizes.
	- encoders tough problem
- Move token decimal scaling to the points which actually transfer the tokens.
	- In my opinion it makes sense, it can be something what we do.
- Remove claim code
	- if we want to keep things simple, we should respect this vision from the start to the end
	- definitely remove claim code in that case!

# Task Priority List
1) First, make a new branch, post-sprearbit-audit
2) Delete claim, pr into audit branch, this fixes one of the criticals
3) Move token decimal scaling to the edges, merge pr into audit branch, this fixes the other critical.
4) Fix the checkInvariant function to remove the decimal scaling over the invariant.
5) Update to 0.8.17 solc

[Alexangelj]

Pushing token scaling to the edges

Native token decimals and WAD values (1e18) are mixed within Portoflio's business logic. One idea is to push the decimal scaling only to the edges, i.e. when tokens are actually transferred.

Lifetime of Token Decimals

• Enter multiprocess
  • Wrap all msg.value in WETH, 18 decimals.
  • Execute logic.
    • If swap, scale amounts to WAD before computing swap related amounts, unscale from WAD before applying to reserves
    • if allocate, no scaling, directly apply to reserves
  • Enter settlement
    • Compute debit and credits.
    • transferFrom if debit > 0
    • Apply to user balance if credit > 0
• Exit multiprocess

So from this, we can see only swap really handles decimal scaling. This probably makes sense to leave in because its siloed to a single function.

[Alexangelj]

VirtualX and VirtualY are tracked in WAD units, but these could be converted to RAY. It would require the scaling in swap to be in RAY as well.