QA Report

[code423n4]

Low Severity Report

(wfCash) Consider using getDepositFromfCashLend function in mintInterval and remove need of refund.

In _mintInternal function, it request to mint an fCashAmount the user specify, with depositAmountExternal amount of collateral, and use _sendTokensToReceiver(token, msg.sender, isETH, balanceBefore); to handle refund and make sure the cash value for account is 0 at the end of the trade.

This pattern can be improved by using the new view function :NotionalV2.getDepositFromfCashLend to calculate exact amount of collateral needed, and only pull that amount of collateral from msg.sender, and remove the code to refund. It can also simplify the logic because the function returns encodedTrade data as well.

This is a better pattern because it doesn't rely on the NotionalV2 protocol to correctly handle refund. If there is an error in Notional, it might be possible for user to lose their funds because of wrong amount of collateral being specified.

it is also more gas efficient because it doesn't need to transfer token back to msg.sender.

Changing the interface to only accept fcashAmount can decrease the attack surface of putting in wrong parameters that might act differently as expected in the underlying protocol.

(wfCash) mint and burn function should return how much collateral is used / released for more efficient integration.

Some efforts were made on the TradeModule to get "how much collateral is used or released", for example in _mintFCashPosition, it needs to cached the balance of sendToken and receiveToken, and make compare the amount after the trade:

(uint256 currentSendTokenBalance,,) = setToken.calculateAndEditDefaultPosition(
    sendToken,
    setTotalSupply,
    preTradeSendTokenBalance
);

(uint256 currentReceiveTokenBalance,,) = setToken.calculateAndEditDefaultPosition(
    receiveToken,
    setTotalSupply,
    preTradeReceiveTokenBalance
);

return (
    preTradeSendTokenBalance.sub(currentSendTokenBalance),
    currentReceiveTokenBalance.sub(preTradeReceiveTokenBalance)
);

This is an inefficient pattern. (Especially given that now both collateral and mintAmount are variables). It would make it a lot easier if the wfCash.mintViaUnderlying and similar functions just return how much collateral were used.

I suggest having this updated with the previous point; let the function calculate what is the collateralAmount, use that amount and also returned it. This will also reduce the attack surface for TradeModule if we reduce the input parameter to only specifying fcashAmount to mint.

[jeffywu]

Interesting suggestion, will review it.