code-423n4 / 2022-06-notional-coop-findings

1 stars 1 forks source link

QA Report #222

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[L-01] Transfering Fcash might trigger postTransferEvent

wfCashLogic.sol#L242-L247

User can chose to receive fcash when exiting from the wfcash. The wfcash would pass userData into the erc1155 data field. This may force the wfcash to execute trade through notional's postTransferEvent. ERC1155Action.sol#L356

Luckily the first parameter of RedeemOpts is boolean, so the first four bytes of userData has to be zero or the transaction would revert.

If the redeemOpts is defined another way, the user may force the wfcash to execute a trade.

    struct RedeemOpts {
        uint32 maxImpliedRate; // user can set maxImpliedRate as the signature.
        address receiver; // this is a receiver field
        bool redeemToUnderlying;
        bool transferfCash;
    }

If the struct is defined as above, the wfcash would be vulnerable.

[L-02] totalAssets of erc4626 should never revert

eip-4626 According to the spec, totalAssets of erc4626 should never revert

MUST NOT revert. wfcash would revert if it's matured but hasn't settled. wfCashERC4626.sol#L23

[L-03] wfcash only approve asset token once

According to the doc, wfcash is implemented in a less efficient way in case asset token is changed.

the ERC4626 standard stipulates a single asset token for the contract. In this case we use the underlying token (i.e. DAI, USDC) rather than the money market token (i.e. cDAI, cUSDC). This choice was made in case the money market token changes in the future, the underlying token will not change in the future. As a consequence, ERC4626 interactions will be less gas efficient.

However, wfcash only approve once. wfCashBase.sol#L68 It stop working if the asset token changed.

Please adjust wfcash accordingly if the asset token may change.

[L-04] mint return the remaning tokens to msg.sender instead of receiver

wfCashLogic.sol#L101 In the _mintInternal function, the remaining tokens is send to the msg.sender instead of the receiver. This is misleading. Recommend to note that in the document.

        _sendTokensToReceiver(token, msg.sender, isETH, balanceBefore);

[N-01] maxMint / maxDeposit is not precise

eip-4626

MUST return the maximum amount of shares mint would allow to be deposited to the receiver and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary). This assumes that the user has infinite assets, i.e. MUST NOT rely on balanceOf of asset.

IT's better to underestimate the max mint amount. The limit of the wfcash should be the liquidity of the notional protocol.

[N-02] convertToShares should not revert.

MUST NOT revert unless due to integer overflow caused by an unreasonably large input.

convertToShares revert when totalAssets is reverted.

jeffywu commented 2 years ago

Good report