The vault exposes a function to withdraw the user's liquidity, the proxy doesn't (and it should).

The current design relies on the flash loan's vault balance as an input source for the minting shares calculations. In a standard vault design where the yield is stored along with the liquidity, this is the correct approach, but it isn't for xycloans' vaults where the yield is stored separately. In the current implementation, one can tamper with the LP shares minting function by transferring funds to the flash loan without depositing them (i.e without also increasing the shares total supply). This is not expected, for the minting function, we should rely on a contract's own storage slot with the total amount of deposits. The withdraw function doesn't necessarily need to use this data and can directly use the flash loan's balance.

After we updated the rewards mechanism it makes no sense to calculate the shares with $$\frac{deposited \cdot totalsupply}{balance}$$

While it could simply be shares = deposited.

https://github.com/xycloo/xycloans/blob/main/factory/src/contract.rs#L32 should be renamed to deploy_pool since vault+flash loan pairs don't exist anymore.

Structures like https://github.com/xycloo/xycloans/blob/main/proxy/src/contract.rs#L108 where initially meant as the foundations for other potential handlings of vault errors, but since we are not going to need them we should revert back to vault.method() rather than vault.try_metohd() + error handling.

We also have some other apparently unnecessary Result returns

We don't use unwraps frequently in our code, but we do at

These aren't potentially dangerous, at least not within the protocol, but it might be better to return clear errors for better interoperation.

The vault unnededly transfers the deposited amount into the vault before transferring it to the flash loan (https://github.com/xycloo/xycloans/blob/main/flash-loan-vault/src/contract.rs#L58)

Yield in a pool is not compound and generated yield for a user does not affect the user's balance in a pool, i.e won't affect the % of the total yield that should go to the user, and this is by design. However, currently, it is permitted to borrow an amount that is the total balance of the pool, which includes also the not-yet-withdrawn fees. Since the borrow occurs in-tx, there is no security risk in leaving this option, but given it is capital that can generate yield without effectively being a user balance/share it might be an ambiguous definition.

We've currently always kept the flash loan and vault contracts separated for a variety of reasons:

• modularity
• code structure
• parallelism (modifying just the flash loan balance not also the vault's balance (eg for fees withdrawals))

But parallelism is not possible anyways in this situation as the flash loan deposits the fees into the vault, zeroing the whole concept of splitting the two contracts.

Also, the cost of instantiating an additional VM should be much larger than the cost for reading a larger WASM module every time:

• for flash loan borrows -> VM instantiated to deposit into vault using Vault::deposit_fees()
• for vault withdrawals -> VM instantiated to call FlashLoan::withdraw()

These two methods should be common enough (especially the flash loan borrows) to make up for the cost of a larger wasm size.

There is a single vault + flash loan pair for every token id. We should add a vector of the existing tokens and extern a function that returns them to make them accessible to other contracts.

We rely on our proxy to forward the calls to other contracts. However, since some vault functions are opened (I can also be accessed without the proxy) for security reasons (users should be able to withdraw from a pool even if the pool was removed from the proxy contract), and most functions would be used by directly calling the specific pool to pay smaller fees.

We could switch to a factory contract design for our proxy, i.e allows to safely deploy and plug pools but won't forward calls. The biggest downside is that this will allow to deposit in pools that aren't connected to the proxy (potentially dangerous).

Overall, I think it might be a good decision, also considering where we're heading for contract upgradeability, but it surely needs further discussion.