asardon
commented
1 year ago Update / Correction
As Jamie pointed out on discord, gauges are pool dependent, ie curve LPs define into which gauge they stake their curve LP tokens. Hence if we want to allow borrowers to use their curve LP tokens for staking in curve -while pledged as collateral in myso- then this requires that the user can define into which curve gauge he wants to stake while initiating a borrow. For this we need to allow users to provide additional staking tx related data (this data is not static, as I incorrectly mentioned before, but instead will be specific to each staking tx).
In light of this, having two dedicated bytes data seems like the only way to go -ie where we have one dedicated bytes data var for passing additional data for callback specific txs (eg swap limits, balancer pool to swap with etc) and one for passing additional data that is specific to compartment related txs (eg which gauge to stake into). By having two separate additional data bytes, we ensure that these two data flows can be handled independently from one another, without potentially conflicting data encodings.
Revised Solution Idea
When a user initiates a borrow we know the collToken address, which in this use case would be a LP curve token. Moreover, as described above the user would also specify which curve gauge he wants to stake into. To ensure that users don’t provide malicious gauge contracts we need to verify the correctness of the provided gauge addr. But instead of maintaining a whitelist for this ourselves (which is hard to keep updated and adds additional gas cost) we can use curve‘s „gauge controller“: https://etherscan.io/address/0x90E00ACe148ca3b23Ac1bC8C240C2a7Dd9c2d7f5#readContract There we can verify that the user provided gauge contract is indeed registered with curve (just need to lookup through gauges getter and for user defined index that result matches). Lastly, we need to check that the gauge addr „belongs“ to given LP token (recall that gauges and pools have a 1:1 relationship), and we can do this via the gauge‘s lp token lookup method to verify that returned lp token address matches the one that user provided (eg see here for stETH-ETH pool https://etherscan.io/address/0x182b723a58739a9c974cfdb385ceadb237453c28#readContract).
https://github.com/mysofinance/v1.2/blob/eb5fd7eca88ad944651c5da4f72c113ab766f72f/contracts/BorrowerGateway.sol#L161
Problem
In the borrower gateway the data bytes are used in two ways: (a) for callback functionality and (b) for compartment creation. This tight coupling (see https://refactoring.guru/refactoring/smells/couplers) can lead to conflicting data usage, ie where callback contract is assuming one form of encoded data (eg minswaplimit, deadline etc for balancer swap) and compartment implementation contract is expecting different encoded data (eg curve staking gauge address). This means that callback contract would need to know about what additional data may be encoded in data depending on compartment logic, which creates an unnecessary dependency among both contracts, where instead it should be possible to use callback/compartment logic independently of one another.
Solution Idea
In Case compartment implementations definitely need dynamic data to be passed, one could add a dedicated compartment data var and rename the callback data accordingly to clearly distinguish between both and allow for targeted usage of either one. Alternatively, one could remove usage of data in compartment logic. Currently compartment logic uses data only in curve staking contract to encode curve gauge address. This address is static and could also be defined as a constant. Note that this usage of data is different than in callback logic, where the dynamic data is tx specific (ie encodes pool id, minswapreceive limit and deadline, which will depend on given tx and asset pair one wants to lever up) and not constant/static (this usage of dynamic data follows the pattern for flashborrows in aave or flashswaps in uni).