search

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

1 stars 1 forks source link

QA Report #215

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Codebase Impressions & Summary

Overall, the contracts are very well implemented and the code quality is very high. Protocol developers provided adequate documentation and information on the protocol.

Running the test suite is extensive and covers most of the code, however, a few things stood out:

Table of Contents

Non-Critical Findings

[NC-01] Use the isETH return value from wfCashBase.getToken instead of checking equality with ETH_ADDRESS

Description

The wfCashBase.getToken returns bool isETH which can be used to figure out if the returned token is the native ETH token.

Findings

NotionalTradeModule.sol#L400-L403

(IERC20 receiveToken,) = fCashPosition.getToken(toUnderlying);
if(address(receiveToken) == ETH_ADDRESS) { // @audit-info use returned `isETH` variable from above function instead
    receiveToken = weth;
}

Recommended mitigation steps

Consider using the returned isETH variable:

(IERC20 receiveToken, bool isETH) = fCashPosition.getToken(toUnderlying);
if(isETH) {
    receiveToken = weth;
}

Low Risk

[L-01] Zero-address checks are missing

Description

Zero-address checks are a best practice for input validation of critical address parameters. While the codebase applies this to most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens, or force redeployment of contracts.

Findings

NotionalTradeModule.sol

L140 - wrappedfCashFactory = _wrappedfCashFactory;\ L141 - weth = _weth;

wfCashBase.sol

L30 - NotionalV2 = _notional;\ L31 - WETH = _weth;

WrappedfCashFactory.sol

L18 - BEACON = _beacon;

Recommended mitigation steps

Add zero-address checks, e.g.:

require(_weth != address(0), "Zero-address");

[L-02] Use of floating pragma

Description

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

https://swcregistry.io/docs/SWC-103

Findings

wfCashERC4626.sol pragma solidity ^0.8.0;

Recommended mitigation steps

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

[L-03] Events not emitted for important state changes

Description

When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Findings

NotionalTradeModule.sol#L301 function setRedeemToUnderlying(ISetToken _setToken, bool _toUnderlying)

Recommended mitigation steps

Emit events for state variable changes.

[L-04] Matured fCash positions not automatically redeemed in NotionalTradeModule.initialize

Description

The function comments imply that matured fCash positions are redeemed within NotionalTradeModule.initialize. However, this redemption is not implemented in this function.

Findings

NotionalTradeModule.sol#L216

* Redeem all fCash positions that have reached maturity for their asset token (cToken)

Recommended mitigation steps

Consider calling the function _redeemMaturedPositions to redeem matured fCash positions or adapt the function comments.

[L-05] Misleading NotionalTradeModule._mintFCashPosition function comments

Description

The function comments imply that the given fCash position is redeemed. However, this function implements minting fCash tokens.

Findings

NotionalTradeModule.sol#L415

* @dev Redeem a given fCash position from the specified send token (either underlying or asset token)

Recommended mitigation steps

Fix the comments to mention minting instead of redeeming.

[L-06] Misleading comment in wfCashLogic._burn function

Description

The comment next to the _withdrawCashToAccount function call implies that the from address is the withdrawal receiver. However, opts.receiver is the receiver.

Findings

wfCashLogic.sol#L230

 // Transfer withdrawn tokens to the `from` address // @audit-info wrong comment - should be `... to the `opts.receiver` address`
  _withdrawCashToAccount(
      currencyId,
      opts.receiver,
      _safeUint88(assetInternalCashClaim),
      opts.redeemToUnderlying
  );

Recommended mitigation steps

Fix the comment.

[L-07] Matured fCash can still be wrapped via ERC1155 transfer

Description

Contrary to the key invariants stated in the README ("After maturity, wrapped fCash can no longer be minted."), matured fCash can be sent to this wfCash contract to receive wrapped fCash tokens in return.

Findings

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashLogic.sol#L107-L152

Recommended mitigation steps

Consider adding a check for fCash maturity:

require(!hasMatured(), "fCash matured");

[L-08] Contracts are using outdated OpenZeppelin version ^3.4.2-solc-0.7

Description

Within notional-wrapped-fcash package.json, an outdated OZ version is used (which has known vulnerabilities, see https://snyk.io/vuln/npm%3A%40openzeppelin%2Fcontracts).

However, as Brownie is used to install dependencies and compile the contracts, using this outdated version declared in the package.json does not impose any risks so far.

Anyway, to prevent any issues in the future (e.g. using solely hardhat to compile and deploy the contracts), upgrade the used OZ packages within the package.json to the latest versions.

See similar findings:

Findings

https://github.com/code-423n4/2022-06-notional-coop/blob/main/notional-wrapped-fcash/package.json#L14

Recommended mitigation steps

Consider using the latest OZ packages within package.json.

[L-09] wfCashERC4626 contract does not conform to EIP4626

Description

The wfCashERC4626 contract implements the EIP4626 standard (EIP-4626: Tokenized Vault Standard).

However, according to EIP4626, the below-mentioned functions do not fully adhere to the specs. They possibly revert due to require checks or revert due to external calls reverting.

Findings

L47 - function totalAssets() public view override returns (uint256)

Possibly reverts due to _getMaturedValue and _getPresentValue reverting.

L52 - function convertToShares(uint256 assets) public view override returns (uint256 shares)

Possibly reverts due to _getPresentValue and totalAssets reverting.

L64 - function convertToAssets(uint256 shares) public view override returns (uint256 assets)

Possibly reverts due to _getPresentValue and totalAssets reverting.

L85 - function maxWithdraw(address owner) public view override returns (uint256)

Possibly reverts due to convertToShares within previewWithdraw reverting.

Recommended mitigation steps

Given the circumstances, in most of the mentioned cases, it's not possible to implement it without ever reverting. Nevertheless, I want to point out that this contract does not fully conform with the EIP4626 standard.

ckoopmann commented 2 years ago

Very nice QA report imo. All the issues regarding the NotionalTradeModule are valid and actionable. Also good format and concise description of the issues and proposed mitigation / fix.

gzeoneth commented 2 years ago

Would move these to Non-critical [L-02] Use of floating pragma [L-03] Events not emitted for important state changes [L-08] Contracts are using outdated OpenZeppelin version ^3.4.2-solc-0.7