Notional x Index Coop
Findings & Analysis Report
2022-07-18
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] fCash of the wrong maturity and asset can be sent to wrapper address before wrapper is deployed
- [M-02] deposit() and mint() and _redeemInternal() in wfCashERC4626() will revert for all fcash that asset token is underlying token because they always call _mintInternal() with useUnderlying==True
- [M-03] The logic of _isUnderlying() in NotionalTradeModule is wrong which will cause mintFCashPosition() and redeemFCashPosition() revert on
fcashtokens which asset token is underlying token (asset.tokenType == TokenType.NonMintable) - [M-04]
IsWrappedFcashcheck is a gas bomb - [M-05] transferfCash does not work as expected
- [M-06] Users Might Not Be Able To Purchase Or Redeem SetToken
- [M-07] Residual Allowance Might Allow Tokens In SetToken To Be Stolen
- [M-08] DOS set token through erc777 hook
- [M-09] Silent overflow of
_fCashAmount - [M-10] User can alter amount returned by redeem function due to control transfer
-
Low Risk and Non-Critical Issues
- Codebase Impressions & Summary
- Table of Contents
- L-01 Zero-address checks are missing
- L-02 Use of floating pragma
- L-03 Events not emitted for important state changes
- L-04 Matured fCash positions not automatically redeemed in
NotionalTradeModule.initialize - L-05 Misleading
NotionalTradeModule._mintFCashPositionfunction comments - L-06 Misleading comment in
wfCashLogic._burnfunction - L-07 Matured fCash can still be wrapped via
ERC1155transfer - L-08 Contracts are using outdated OpenZeppelin version
^3.4.2-solc-0.7 - L-09
wfCashERC4626contract does not conform toEIP4626 - N-01 Use the
isETHreturn value fromwfCashBase.getTokeninstead of checking equality withETH_ADDRESS
-
- 1 Avoid contract existence checks by using solidity version 0.8.10 or later
- 2 Multiple accesses of a mapping/array should use a local variable cache
- 3
internalfunctions only called once can be inlined to save gas - 4
<array>.lengthshould not be looked up in every loop of afor-loop - 5
require()/revert()strings longer than 32 bytes cost extra gas - 6
privatefunctions not called by the contract should be removed to save deployment gas - 7 Optimize names to save gas
- 8 Using
bools for storage incurs overhead - 9 Use a more recent version of solidity
- 10 Use a more recent version of solidity
- 11 It costs more gas to initialize non-
constant/non-immutablevariables to zero than to let the default of zero be applied - 12
++icosts less gas thani++, especially when it’s used infor-loops (--i/i--too) - 13 Splitting
require()statements that use&&saves gas - 14 Usage of
uints/intssmaller than 32 bytes (256 bits) incurs overhead - 15
abi.encode()is less efficient thanabi.encodePacked() - 16 Using
privaterather thanpublicfor constants, saves gas - 17 Use custom errors rather than
revert()/require()strings to save gas - 18 Functions guaranteed to revert when called by normal users can be marked
payable
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the Notional x Index Coop smart contract system written in Solidity. The audit contest took place between June 7—June 14 2022.
Wardens
81 Wardens contributed reports to the Notional x Index Coop:
- jonah1005
- unforgiven
- xiaoming90
- csanuragjain
- 0xDjango
- berndartmueller
- GreyArt (hickuphh3 and itsmeSTYJ)
- Meera
- minhquanym
- kenzo
- 0x52
- IllIllI
- antonttc
- GimelSec (rayn and sces60107)
- sorrynotsorry
- joestakey
- 0xkatana
- 0x29A (0x4non and rotcivegaf)
- 0x1f8b
- 0xf15ers (remora and twojoy)
- Funen
- hyh
- PierrickGT
- Chom
- delfin454000
- Waze
- Picodes
- Deivitto
- fatherOfBlocks
- 0xKitsune
- TomJ
- TerrierLover
- simon135
- _Adam
- slywaters
- oyc_109
- ellahi
- saian
- sach1r0
- catchup
- c3phas
- Sm4rty
- Cityscape
- 0xNazgul
- hake
- 0xmint
- Tadashi
- Lambda
- hansfriese
- Ruhum
- zzzitron
- Trumpero
- sseefried
- cloudjunky
- cccz
- Bronicle
- Nethermind
- dipp
- cryptphi
- 0xNineDec
- z3s
- JC
- ayeslick
- Tomio
- rfa
- 8olidity
- 0xSolus
- UnusualTurtle
- asutorufos
- samruna
- kaden
- ElKu
- DavidGialdi
- 0v3rf10w
- ynnad
- Fitraldys
- djxploit
This contest was judged by gzeon.
Final report assembled by itsmetechjay.
Summary
The C4 analysis yielded an aggregated total of 11 unique vulnerabilities. Of these vulnerabilities, 1 received a risk rating in the category of HIGH severity and 10 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 61 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 55 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Notional x Index Coop repository, and is composed of 5 smart contracts written in the Solidity programming language and includes 914 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
High Risk Findings (1)
[H-01] Rounding Issues In Certain Functions
Submitted by xiaoming90, also found by berndartmueller, GreyArt, jonah1005, kenzo, and minhquanym
Background
Per EIP 4626’s Security Considerations (https://eips.ethereum.org/EIPS/eip-4626)
Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:
- If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.
- If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.
Thus, the result of the previewMint and previewWithdraw should be rounded up.
Proof of Concept
The current implementation of convertToShares function will round down the number of shares returned due to how solidity handles Integer Division. ERC4626 expects the returned value of convertToShares to be rounded down. Thus, this function behaves as expected.
function convertToShares(uint256 assets) public view override returns (uint256 shares) {
uint256 supply = totalSupply();
if (supply == 0) {
// Scales assets by the value of a single unit of fCash
uint256 unitfCashValue = _getPresentValue(uint256(Constants.INTERNAL_TOKEN_PRECISION));
return (assets * uint256(Constants.INTERNAL_TOKEN_PRECISION)) / unitfCashValue;
}
return (assets * totalSupply()) / totalAssets();
}ERC 4626 expects the result returned from previewWithdraw function to be rounded up. However, within the previewWithdraw function, it calls the convertToShares function. Recall earlier that the convertToShares function returned a rounded down value, thus previewWithdraw will return a rounded down value instead of round up value. Thus, this function does not behave as expected.
function previewWithdraw(uint256 assets) public view override returns (uint256 shares) {
if (hasMatured()) {
shares = convertToShares(assets);
} else {
// If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
(uint16 currencyId, uint40 maturity) = getDecodedID();
(shares, /* */, /* */) = NotionalV2.getfCashBorrowFromPrincipal(
currencyId,
assets,
maturity,
0,
block.timestamp,
true
);
}
}previewWithdraw and previewMint functions rely on NotionalV2.getfCashBorrowFromPrincipal and NotionalV2.getDepositFromfCashLend functions. Due to the nature of time-boxed contest, I was unable to verify if NotionalV2.getfCashBorrowFromPrincipal and NotionalV2.getDepositFromfCashLend functions return a rounded down or up value. If a rounded down value is returned from these functions, previewWithdraw and previewMint functions would not behave as expected.
Impact
Other protocols that integrate with Notional’s fCash wrapper might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some intergration problem in the future that can lead to wide range of issues for both parties.
Recommended Mitigation Steps
Ensure that the rounding of vault’s functions behave as expected. Following are the expected rounding direction for each vault function:
- previewMint(uint256 shares) - Round Up ⬆
- previewWithdraw(uint256 assets) - Round Up ⬆
- previewRedeem(uint256 shares) - Round Down ⬇
- previewDeposit(uint256 assets) - Round Down ⬇
- convertToAssets(uint256 shares) - Round Down ⬇
- convertToShares(uint256 assets) - Round Down ⬇
previewMint returns the amount of assets that would be deposited to mint specific amount of shares. Thus, the amount of assets must be rounded up, so that the vault won’t be shortchanged.
previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the vault won’t be shortchanged.
Following is the OpenZeppelin’s vault implementation for rounding reference:
Alternatively, if such alignment of rounding could not be achieved due to technical limitation, at the minimum, document this limitation in the comment so that the developer performing the integration is aware of this.
gzeoneth (judge) increased severity to High and commented:
Judging this and all duplicate regarding EIP4626 implementation as High Risk.
EIP4626 is aimed to create a consistent and robust implementation patterns for Tokenized Vaults. A slight deviation from 4626 would broke composability and potentially lead to loss of fund (POC in https://github.com/code-423n4/2022-06-notional-coop-findings/issues/88 can be an example). It is counterproductive to implement EIP4626 but does not conform to it fully. Especially it does seem that most of the time
depositwould be successful but notwithdraw, making it even more dangerous when an immutable consumer application mistakenly used the wfcash contract.
Medium Risk Findings (10)
[M-01] fCash of the wrong maturity and asset can be sent to wrapper address before wrapper is deployed
Submitted by 0x52, also found by jonah1005 and unforgiven
Minting becomes impossible
Proof of Concept
onERC1155Received is only called when the size of the code deployed at the address contains code. Since create2 is used to deploy the contract, the address can be calculated before the contract is deployed. A malicious actor could send the address fCash of a different maturity or asset before the contract is deployed and since nothing has been deployed, onERC1155Received will not be called and the address will accept the fCash. After the contract is deployed and correct fCash is sent to the address, onERC1155Received will check the length of the assets held by the address and it will be more than 1 (fCash of correct asset and maturity and fCash with wrong maturity or asset sent before deployment). This will cause the contract to always revert essentially breaking the mint completely.
Recommended Mitigation Steps
When the contract is created create a function that reads how many fCash assets are at the address and send them away if they aren’t of the correct asset and maturity
jeffywu (Notional) confirmed, but disagreed with severity and commented:
I will need to write a PoC to confirm that this is the case but it seems plausible to me.
Based on the Judging Criteria, this does not result in loss of funds. This will result in a loss of availability (available funds actually increase).
My opinion is medium severity.
gzeoneth (judge) decreased severity to Medium and commented:
Judging this as Med Risk due to no loss of funds and only possible before contract deployment.
[M-02] deposit() and mint() and _redeemInternal() in wfCashERC4626() will revert for all fcash that asset token is underlying token because they always call _mintInternal() with useUnderlying==True
Submitted by unforgiven
Impact
For some fcash the asset token is underlying token (asset.tokenType == TokenType.NonMintable) and NotionalV2 will not handle minting with useUnderlying==True for those fcashs (according to what I asked from sponsor). In summery most of the logics in wfCashERC4626 will not work for those fcash tokens.
when for some fcash asset token is underlying token, all calls to NotionalV2 should be with useUnderlying==False. but deposit() and mint() in wfCashERC4626 contract call _mintInternal() with useUnderlying==True and it calls NotionalV2.batchLend() with depositUnderlying==true so the NotionV2 call will fail for fcash tokens that asset token is underlying token and it would cause that deposit() and mint() logic wfCashERC4626 will not work and contract will be useless for those tokens.
_redeemInternal() issue is similar and it calls _burn() with redeemToUnderlying: true which execution eventually calls NotionalV2.batchBalanceAndTradeAction() with toUnderlying=True which will revert so _redeemInternal() will fail and because withdraw() and redeem use it, so they will not work too for those fcash tokens that asset token is underlying token.
Proof of Concept
This is deposit() and mint() code in wfCashERC4626:
/** @dev See {IERC4626-deposit} */
function deposit(uint256 assets, address receiver) public override returns (uint256) {
uint256 shares = previewDeposit(assets);
// Will revert if matured
_mintInternal(assets, _safeUint88(shares), receiver, 0, true);
emit Deposit(msg.sender, receiver, assets, shares);
return shares;
}
/** @dev See {IERC4626-mint} */
function mint(uint256 shares, address receiver) public override returns (uint256) {
uint256 assets = previewMint(shares);
// Will revert if matured
_mintInternal(assets, _safeUint88(shares), receiver, 0, true);
emit Deposit(msg.sender, receiver, assets, shares);
return assets;
}As you can see they both call _mintInternal() with last parameter as true which is useUnderlying’s value. This is _mintInternal() code:
function _mintInternal(
uint256 depositAmountExternal,
uint88 fCashAmount,
address receiver,
uint32 minImpliedRate,
bool useUnderlying
) internal nonReentrant {
require(!hasMatured(), "fCash matured");
(IERC20 token, bool isETH) = getToken(useUnderlying);
uint256 balanceBefore = isETH ? address(this).balance : token.balanceOf(address(this));
// If dealing in ETH, we use WETH in the wrapper instead of ETH. NotionalV2 uses
// ETH natively but due to pull payment requirements for batchLend, it does not support
// ETH. batchLend only supports ERC20 tokens like cETH or aETH. Since the wrapper is a compatibility
// layer, it will support WETH so integrators can deal solely in ERC20 tokens. Instead of using
// "batchLend" we will use "batchBalanceActionWithTrades". The difference is that "batchLend"
// is more gas efficient (does not require and additional redeem call to asset tokens). If using cETH
// then everything will proceed via batchLend.
if (isETH) {
IERC20((address(WETH))).safeTransferFrom(msg.sender, address(this), depositAmountExternal);
WETH.withdraw(depositAmountExternal);
BalanceActionWithTrades[] memory action = EncodeDecode.encodeLendETHTrade(
getCurrencyId(),
getMarketIndex(),
depositAmountExternal,
fCashAmount,
minImpliedRate
);
// Notional will return any residual ETH as the native token. When we _sendTokensToReceiver those
// native ETH tokens will be wrapped back to WETH.
NotionalV2.batchBalanceAndTradeAction{value: depositAmountExternal}(address(this), action);
} else {
// Transfers tokens in for lending, Notional will transfer from this contract.
token.safeTransferFrom(msg.sender, address(this), depositAmountExternal);
// Executes a lending action on Notional
BatchLend[] memory action = EncodeDecode.encodeLendTrade(
getCurrencyId(),
getMarketIndex(),
fCashAmount,
minImpliedRate,
useUnderlying
);
NotionalV2.batchLend(address(this), action);
}
// Mints ERC20 tokens for the receiver, the false flag denotes that we will not do an
// operatorAck
_mint(receiver, fCashAmount, "", "", false);
_sendTokensToReceiver(token, msg.sender, isETH, balanceBefore);
}As you can see it calls NotionalV2 functions with useUnderlying=True but according to sponsor clarification NotionalV2 would fail and revert for those calls because useUnderlying=True and fcash’s asset token is underlying token (asset.tokenType == TokenType.NonMintable).
So in summery for fcash tokens which asset token is underlying token NotionalV2 won’t handle calls which include useUnderlying==True but in wfCashERC4626 contract functions like deposit(), mint(), withdraw() and redeem() they all uses useUnderlying==True always so wfCashERC4626 won’t work for those specific type of tokens which asset token is underlying token(asset.tokenType == TokenType.NonMintable)
the detail explanations for functions withdraw() and redeem() are similar.
Tools Used
VIM
Recommended Mitigation Steps
Check that if for that fcash token asset token is underlying token or not and set useUnderlying based on that.
gzeoneth (judge) decreased severity to Medium and commented:
There doesn’t seems to be loss of fund as
depositandmintwould revert. Judging as Med Risk.
[M-03] The logic of _isUnderlying() in NotionalTradeModule is wrong which will cause mintFCashPosition() and redeemFCashPosition() revert on fcash tokens which asset token is underlying token (asset.tokenType == TokenType.NonMintable)
Submitted by unforgiven
For some fcash the asset token is underlying token (asset.tokenType == TokenType.NonMintable) and NotionalV2 will not handle minting or burning when it is called with useUnderlying==True for those fcashs (according to what I asked from sponsor). In summery most of the logics in NotionalTradeModule will not work for those fcash tokens because _isUnderlying() returns true result for those tokens which would make NotionalTradeModule’s logic for mintFCashPosition() and redeemFCashPosition() will eventually call redeemToUnderlying() and mintViaUnderlying() in wfCashLogic and those function in wfCashLogic will call NotionalV2 with useUnderlying==True and NotionalV2 will fail and revert for fcash tokens which asset token is underlying token, so the whole transaction will fail and _mintFCashPosition() and _redeemFCashPosition() logic in NotionalTradeModule will not work for those fcash tokens and manager can’t add them to set protocol.
Proof of Concept
when for some fcash asset token is underlying token, all calls to NotionalV2 should be with useUnderlying==False. but _isUnderlying() in NotionalTradeModule contract first check that isUnderlying = _paymentToken == underlyingToken so for fcash tokens where asset token is underlying token it is going to return isUnderlying==True. let’s assume that for some specific fcash asset token is underlying token (asset.tokenType == TokenType.NonMintable) and follow the code execution.
This is _isUnderlying() code in NotionalTradeModule:
function _isUnderlying(
IWrappedfCashComplete _fCashPosition,
IERC20 _paymentToken
)
internal
view
returns(bool isUnderlying)
{
(IERC20 underlyingToken, IERC20 assetToken) = _getUnderlyingAndAssetTokens(_fCashPosition);
isUnderlying = _paymentToken == underlyingToken;
if(!isUnderlying) {
require(_paymentToken == assetToken, "Token is neither asset nor underlying token");
}
}As you can see it calls _getUnderlyingAndAssetTokens() and then check _paymentToken == underlyingToken to see that if payment token is equal to underlyingToken. _getUnderlyingAndAssetTokens() uses getUnderlyingToken() and getAssetToken() in wfCashBase. This is getUnderlyingToken() code in wfCashBase:
/// @notice Returns the token and precision of the token that this token settles
/// to. For example, fUSDC will return the USDC token address and 1e6. The zero
/// address will represent ETH.
function getUnderlyingToken() public view override returns (IERC20 underlyingToken, int256 underlyingPrecision) {
(Token memory asset, Token memory underlying) = NotionalV2.getCurrency(getCurrencyId());
if (asset.tokenType == TokenType.NonMintable) {
// In this case the asset token is the underlying
return (IERC20(asset.tokenAddress), asset.decimals);
} else {
return (IERC20(underlying.tokenAddress), underlying.decimals);
}
}As you can see for our specific fcash token this function will return asset token as underlying token. so for this specific fcash token, the asset token and underlying token will be same in _isUnderlying() of NationalTradeModule but because code first check isUnderlying = _paymentToken == underlyingToken so the function will return isUnderlying=True as a result for our specific fcash token (which asset token is underlying token)
This is _mintFCashPosition() and _redeemFCashPosition() code in NotionalTradeModule:
/**
* @dev Redeem a given fCash position from the specified send token (either underlying or asset token)
* @dev Alo adjust the components / position of the set token accordingly
*/
function _mintFCashPosition(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _sendToken,
uint256 _fCashAmount,
uint256 _maxSendAmount
)
internal
returns(uint256 sentAmount)
{
if(_fCashAmount == 0) return 0;
bool fromUnderlying = _isUnderlying(_fCashPosition, _sendToken);
_approve(_setToken, _fCashPosition, _sendToken, _maxSendAmount);
uint256 preTradeSendTokenBalance = _sendToken.balanceOf(address(_setToken));
uint256 preTradeReceiveTokenBalance = _fCashPosition.balanceOf(address(_setToken));
_mint(_setToken, _fCashPosition, _maxSendAmount, _fCashAmount, fromUnderlying);
(sentAmount,) = _updateSetTokenPositions(
_setToken,
address(_sendToken),
preTradeSendTokenBalance,
address(_fCashPosition),
preTradeReceiveTokenBalance
);
require(sentAmount <= _maxSendAmount, "Overspent");
emit FCashMinted(_setToken, _fCashPosition, _sendToken, _fCashAmount, sentAmount);
}
/**
* @dev Redeem a given fCash position for the specified receive token (either underlying or asset token)
* @dev Alo adjust the components / position of the set token accordingly
*/
function _redeemFCashPosition(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _receiveToken,
uint256 _fCashAmount,
uint256 _minReceiveAmount
)
internal
returns(uint256 receivedAmount)
{
if(_fCashAmount == 0) return 0;
bool toUnderlying = _isUnderlying(_fCashPosition, _receiveToken);
uint256 preTradeReceiveTokenBalance = _receiveToken.balanceOf(address(_setToken));
uint256 preTradeSendTokenBalance = _fCashPosition.balanceOf(address(_setToken));
_redeem(_setToken, _fCashPosition, _fCashAmount, toUnderlying);
(, receivedAmount) = _updateSetTokenPositions(
_setToken,
address(_fCashPosition),
preTradeSendTokenBalance,
address(_receiveToken),
preTradeReceiveTokenBalance
);
require(receivedAmount >= _minReceiveAmount, "Not enough received amount");
emit FCashRedeemed(_setToken, _fCashPosition, _receiveToken, _fCashAmount, receivedAmount);
}As you can see they both uses _isUnderlying() to find out that if _sendToken is asset token or underlying token. for our specific fcash token, the result of _isUnderlying() will be True and _mintFCashPosition() and _redeemFCashPosition() will call _mint() and _redeem() with toUnderlying set as True. This is _mint() and _redeem() code:
/**
* @dev Invokes the wrappedFCash token's mint function from the setToken
*/
function _mint(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
uint256 _maxAssetAmount,
uint256 _fCashAmount,
bool _fromUnderlying
)
internal
{
uint32 minImpliedRate = 0;
bytes4 functionSelector =
_fromUnderlying ? _fCashPosition.mintViaUnderlying.selector : _fCashPosition.mintViaAsset.selector;
bytes memory mintCallData = abi.encodeWithSelector(
functionSelector,
_maxAssetAmount,
uint88(_fCashAmount),
address(_setToken),
minImpliedRate,
_fromUnderlying
);
_setToken.invoke(address(_fCashPosition), 0, mintCallData);
}
/**
* @dev Redeems the given amount of fCash token on behalf of the setToken
*/
function _redeem(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
uint256 _fCashAmount,
bool _toUnderlying
)
internal
{
uint32 maxImpliedRate = type(uint32).max;
bytes4 functionSelector =
_toUnderlying ? _fCashPosition.redeemToUnderlying.selector : _fCashPosition.redeemToAsset.selector;
bytes memory redeemCallData = abi.encodeWithSelector(
functionSelector,
_fCashAmount,
address(_setToken),
maxImpliedRate
);
_setToken.invoke(address(_fCashPosition), 0, redeemCallData);
}As you can see they are using _toUnderlying value to decide calling between (mintViaUnderlying() or mintViaAsset()) and (redeemToUnderlying() or redeemToAsset()), for our specific fcash _toUnderlying will be True so those functions will call mintViaUnderlying() and redeemToUnderlying() in wfCashLogic.
mintViaUnderlying() and redeemToUnderlying() in wfCashLogic execution flow eventually would call NotionalV2 functions with useUnderlying=True for this specific fcash token, but NotionalV2 will revert for that call because for that fcash token asset token is underlying token and NotionalV2 can’t handle calls with useUnderlying==True for that fcash Token. This will cause all the transaction to fail and manager can’t call redeemFCashPosition() or mintFCashPosition() functions for those fcash tokens that asset token is underlying token.
In summery NotionalTradeModule logic will not work for all fcash tokens becasue the logic of _isUnderlying() is wrong for fcash tokens that asset token is underlying token.
Tools Used
VIM
Recommended Mitigation Steps
Change the logic of _isUnderlying() in NotionalTradeModule so it returns correct results for all fcash tokens. One simple solution can be that it first check payment token value with asset token value.
ckoopmann (Index Coop) confirmed, but disagreed with severity and commented:
Will need input from @jeffywu here, is the description of the Notional side of things correct ?
I’ll also try to reproduce this issue in a test maybe to make sure if it is valid.
Not sure if this is “High Risk” as no funds seem to be at risk. However from the description it might render the
NotionalTradeModuleincompatible with certain fCash tokens, which we certainly want to avoid. So will have to review this in more detail.
Agree with @ckoopmann that severity should be reduced here, what this would cause is a revert not any loss of funds.
The simple fix would just be to always use
mintViaAssetandmintViaUnderlyingfor the case when fCash has a non-mintable type (currently there are no fCash assets of this type and none planned). However, we can also add the ability to query this on the wfCash side to make things more compatible.
ckoopmann (Index Coop) commented:
@jeffywu What do you think of the suggested mitigation strategy ?
As far as I understand we would just have to change:
isUnderlying = _paymentToken == underlyingToken;toisUnderlying = _paymentToken != assetToken;to avoid the described issue, no ?
To make this more fool proof what I can do is just add a check on the wrapped fCash side to see if the token is non-mintable and then override the useUnderlying flag internally there. I think that will be a better solution since getUnderlyingToken already has logic to return the asset token when it is marked as non-mintable. That would mean that you would not need to make any changes, I think this will make it easier for integrating developers in the future.
Specifically add checks here: https://github.com/notional-finance/wrapped-fcash/blob/master/contracts/wfCashLogic.sol#L57-L58 and here: https://github.com/notional-finance/wrapped-fcash/blob/master/contracts/wfCashLogic.sol#L210-L211
and overwrite the incoming useUnderlying if we are in this situation.
ckoopmann (Index Coop) commented:
@jeffywu Sounds great to me. If I understand correctly that means I would not have to change anything on the trade module side. Let me know if that is incorrect.
jeffywu (Notional) resolved and commented:
@ckoopmann, no changes necessary on your side. You can see the changes here.
gzeoneth (judge) decreased severity to Medium and commented:
There doesn’t seems to be loss of fund as it would revert. Judging as Med Risk.
[M-04] IsWrappedFcash check is a gas bomb
Submitted by jonah1005
In the _isWrappedFCash check, the notionalTradeModule check whether the component is a wrappedCash with the following logic.
try IWrappedfCash(_fCashPosition).getDecodedID() returns(uint16 _currencyId, uint40 _maturity){
try wrappedfCashFactory.computeAddress(_currencyId, _maturity) returns(address _computedAddress){
return _fCashPosition == _computedAddress;
} catch {
return false;
}
} catch {
return false;
}The above logic is dangerous when _fCashPosition do not revert on getDecodedID but instead give a wrong format of return value. The contract would try to decode the return value into returns(uint16 _currencyId, uint40 _maturity) and revert. The revert would consume what ever gas it’s provided.
CETH is an example.
There’s a fallback function in ceth
function () external payable {
requireNoError(mintInternal(msg.value), "mint failed");
}As a result, calling getDecodedID would not revert. Instead, calling getDecodedID of CETH would consume all remaining gas.
This creates so many issues. First, users would waste too much gas on a regular operation. Second, the transaction might fail if ceth is not the last position. Third, the wallet contract can not interact with set token with ceth as it consumes all gas.
Proof of Concept
The following contract may fail to redeem setTokens as it consumes too much gas (with 20M gas limit).
function test(uint256 _amount) external {
cToken.approve(address(issueModule), uint256(-1));
wfCash.approve(address(issueModule), uint256(-1));
issueModule.issue(setToken, _amount, address(this));
issueModule.redeem(setToken, _amount, address(this));
}Also, we can check how much gas it consumes with the following function.
function TestWrappedFCash(address _fCashPosition) public view returns(bool){
if(!_fCashPosition.isContract()) {
return false;
}
try IWrappedfCash(_fCashPosition).getDecodedID() returns(uint16 _currencyId, uint40 _maturity){
try wrappedfCashFactory.computeAddress(_currencyId, _maturity) returns(address _computedAddress){
return _fCashPosition == _computedAddress;
} catch {
return false;
}
} catch {
return false;
}
}Test this function with cdai and ceth, we can observe that there’s huge difference of gas consumption here.
Gas used: 30376 of 130376
Gas used: 19479394 of 19788041Tools Used
Hardhat
Recommended Mitigation Steps
I recommend building a map in the notionalTradeModule and inserting the wrappeCash in the mintFCashPosition function.
function addWrappedCash(uint16 _currencyId, uint40 _maturity) public {
address computedAddress = wrappedfCashFactory.computeAddress(_currencyId, _maturity);
wrappedFCash[computedAddress] = true;
}Or we could replace the try-catch pattern with a low-level function call and check the return value’s length before decoding it.
Something like this might be a fix.
(bool success, bytes memory returndata) = target.delegatecall(data);
if (!success || returndata.length != DECODED_ID_RETURN_LENGTH) {
return false;
}
// abi.decode ....ckoopmann (Index Coop) confirmed and commented:
Correct, this is an issue that I also recently ran into (after the contest had already started) when doing additional tests. My solution was to just add a fixed gas limit to the
getDecodedIDcall which seemed to solve it.In an earlier version of the contract I had a manual mapping as suggested here, however this is not ideal since the setToken could obtain fCash positions using other SetModules (such as the general TradeModule) which would then not be registered in this mapping.
Limiting the gas usage of these calls seems like an easier and more robust mitigation strategy. (might want to make these gas limits updateable though)
gzeoneth (judge) decreased severity to Medium and commented:
Valid but don’t think this is High Risk,
eth_estimateGasshould fail preventing most user from interacting with a ridiculous gas limit.
[M-05] transferfCash does not work as expected
Submitted by csanuragjain
If maturity is reached and user has asked for redeem with opts.transferfCash as true, then if (hasMatured()) turns true at wfCashLogic.sol#L216 causing fcash to be cashed out in underlying token and then sent to receiver. So receiver obtains underlying when fcash was expected. The sender wont get an error thinking fcash transfer was success
Proof of Concept
- User A calls redeem with opts.transferfCash as true and receiver as User B
- Since maturity is reached so instead of transferring the fCash, contract would simply cash out fCash and sent the underlying token to the receiver which was not expected
Recommended Mitigation Steps
If opts.transferfCash is true and maturity is reached then throw an error mentioning that fCash can no longer be transferred
Sounds reasonable.
[M-06] Users Might Not Be Able To Purchase Or Redeem SetToken
Submitted by xiaoming90, also found by berndartmueller
Proof of Concept
Whenever a setToken is issued or redeemed, the moduleIssueHook and moduleRedeemHook will be triggered. These two hooks will in turn call the _redeemMaturedPositions function to ensure that no matured fCash positions remain in the Set by redeeming any matured fCash position.
/**
* @dev Hook called once before setToken issuance
* @dev Ensures that no matured fCash positions are in the set when it is issued
*/
function moduleIssueHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
_redeemMaturedPositions(_setToken);
}
/**
* @dev Hook called once before setToken redemption
* @dev Ensures that no matured fCash positions are in the set when it is redeemed
*/
function moduleRedeemHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
_redeemMaturedPositions(_setToken);
}The _redeemMaturedPositions will loop through all its fCash positions and attempts to redeem any fCash position that has already matured. However, if one of the fCash redemptions fails, it will cause the entire function to revert. If this happens, no one could purchase or redeem the setToken because moduleIssueHook and modileRedeemHook hooks will revert every single time. Thus, the setToken issuance and redemption will stop working entirely and this setToken can be considered “bricked”.
/**
* @dev Redeem all matured fCash positions for the given SetToken
*/
function _redeemMaturedPositions(ISetToken _setToken)
internal
{
ISetToken.Position[] memory positions = _setToken.getPositions();
uint positionsLength = positions.length;
bool toUnderlying = redeemToUnderlying[_setToken];
for(uint256 i = 0; i < positionsLength; i++) {
// Check that the given position is an equity position
if(positions[i].unit > 0) {
address component = positions[i].component;
if(_isWrappedFCash(component)) {
IWrappedfCashComplete fCashPosition = IWrappedfCashComplete(component);
if(fCashPosition.hasMatured()) {
(IERC20 receiveToken,) = fCashPosition.getToken(toUnderlying);
if(address(receiveToken) == ETH_ADDRESS) {
receiveToken = weth;
}
uint256 fCashBalance = fCashPosition.balanceOf(address(_setToken));
_redeemFCashPosition(_setToken, fCashPosition, receiveToken, fCashBalance, 0);
}
}
}
}
}Impact
User will not be able to purchase or redeem the setToken. User’s fund will be stuck in the SetToken Contract. Unable to remove matured fCash positions from SetToken and update positions of its asset token.
Recommended Mitigation Steps
This is a problem commonly encountered whenever a method of a smart contract calls another contract – you cannot rely on the other contract to work 100% of the time, and it is dangerous to assume that the external call will always be successful.
It is recommended to:
- Consider alternate method of updating the asset position so that the SetToken’s core functions (e.g. issuance and redemption) will not be locked if one of the matured fCash redemptions fails.
- Evaluate if
_redeemMaturedPositionsreally need to be called during SetToken’s issuance and redemption. If not, consider removing them from the hooks, so that any issue or revert within_redeemMaturedPositionswon’t cause the SetToken’s issuance and redemption functions to stop working entirely. - Consider implementing additional function to give manager/user an option to specify a list of matured fCash positions to redeem instead of forcing them to redeem all matured fCash positions at one go.
ckoopmann (Index Coop) acknowledged, but disagreed with severity and commented:
The
_isWrappedFCash(component)should make sure that these calls are only executed on valid wrappedfCash instances deployed from the configured factory.The issue does not outline a practical scenario / test case where this issue would actually arise. If it did the manager could probably still remove / redeem the component either via this module, or one of the other Trade modules.
However I’ll have to look into this if I can find a scenario where this would actually arise. I will also consider making this redeem step optional, but I will have to think more about this as this might introduce other more serious issues.
@jeffywu Do you see any scenario where the redemption of a matured fCash position might fail in this context`?
EDIT: Just noticed that https://github.com/code-423n4/2022-06-notional-coop-findings/issues/226 mentions (among others) the scenario of USDC tokens being blocked which I guess is unlikely but outside of our control. This makes me think we might want to make the redemption of matured tokens during issuance / redemption optional.
ckoopmann (Index Coop) confirmed and commented:
Changed label from acknowledged to confirmed, since on second thought I think we likely want to adopt some kind of mitigation strategy for this. However still tentative / unclear which strategy we want to adopt.
[M-07] Residual Allowance Might Allow Tokens In SetToken To Be Stolen
Submitted by xiaoming90
Proof of Concept
Whenever _mintFCashPosition function is called to mint new fCash position, the contract will call the _approve function to set the allowance to _maxSendAmount so that the fCash Wrapper contact can pull the payment tokens from the SetToken contract during minting.
function _mintFCashPosition(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _sendToken,
uint256 _fCashAmount,
uint256 _maxSendAmount
)
internal
returns(uint256 sentAmount)
{
if(_fCashAmount == 0) return 0;
bool fromUnderlying = _isUnderlying(_fCashPosition, _sendToken);
_approve(_setToken, _fCashPosition, _sendToken, _maxSendAmount);
uint256 preTradeSendTokenBalance = _sendToken.balanceOf(address(_setToken));
uint256 preTradeReceiveTokenBalance = _fCashPosition.balanceOf(address(_setToken));
_mint(_setToken, _fCashPosition, _maxSendAmount, _fCashAmount, fromUnderlying)
..SNIP..
}Note that _maxSendAmount is the maximum amount of payment tokens that is allowed to be consumed during minting. This is not the actual amount of payment tokens consumed during the minting process. Thus, after the minting, there will definitely be some residual allowance since it is unlikely that the fCash wrapper contract will consume the exact maximum amount during minting.
The following piece of code shows that having some residual allowance is expected. The _approve function will not set the allowance unless there is insufficient allowance.
/**
* @dev Approve the given wrappedFCash instance to spend the setToken's sendToken
*/
function _approve(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _sendToken,
uint256 _maxAssetAmount
)
internal
{
if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
_setToken.invoke(address(_sendToken), 0, approveCallData);
}
}Impact
Having residual allowance increases the risk of the asset tokens being stolen from the SetToken contract. SetToken contract is where all the tokens/assets are held. If the Notional’s fCash wrapper contract is compromised, it will allow the compromised fCash wrapper contract to withdraw funds from the SetToken contract due to the residual allowance.
Note that Notional’s fCash wrapper contract is not totally immutable, as it is a upgradeable contract. This is an additional risk factor to be considered. If the Notional’s deployer account is compromised, the attacker could upgrade the Notional’s fCash wrapper contract to a malicious one to withdraw funds from the Index Coop’s SetToken contract due to the residual allowance.
Index Coop and Notional are two separate protocols and teams. Thus, it is a good security practice not to place any trust on external party wherever possible to ensure that if one party is compromised, it won’t affect the other party. Thus, there should not be any residual allowance that allows Notional’s contract to withdraw funds from Index Coop’s contract in any circumstance.
In the worst case scenario, a “lazy” manager might simply set the _maxAssetAmount to type(uint256).max. Thus, this will result in large amount of residual allowance left, and expose the SetToken contract to significant risk.
Recommended Mitigation Steps
Approve the allowance on-demand whenever _mintFCashPosition is called, and reset the allowance back to zero after each minting process to eliminate any residual allowance.
function _mintFCashPosition(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _sendToken,
uint256 _fCashAmount,
uint256 _maxSendAmount
)
internal
returns(uint256 sentAmount)
{
if(_fCashAmount == 0) return 0;
bool fromUnderlying = _isUnderlying(_fCashPosition, _sendToken);
_approve(_setToken, _fCashPosition, _sendToken, _maxSendAmount);
uint256 preTradeSendTokenBalance = _sendToken.balanceOf(address(_setToken));
uint256 preTradeReceiveTokenBalance = _fCashPosition.balanceOf(address(_setToken));
_mint(_setToken, _fCashPosition, _maxSendAmount, _fCashAmount, fromUnderlying)
..SNIP..
+ // Reset the allowance back to zero after minting
+ _approve(_setToken, _fCashPosition, _sendToken, 0);
}Update the _approve accordingly to remove the if-statement related to residual allowance.
function _approve(
ISetToken _setToken,
IWrappedfCashComplete _fCashPosition,
IERC20 _sendToken,
uint256 _maxAssetAmount
)
internal
{
- if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
_setToken.invoke(address(_sendToken), 0, approveCallData);
- }
}ckoopmann (Index Coop) confirmed and commented:
This looks like a prudent suggestion. Will review and potentially adopt the suggested mitigation.
[M-08] DOS set token through erc777 hook
Submitted by jonah1005
Impact
The wfCash is an erc777 token. ERC777.sol#L376-L380 Users can get the control flow before sending token and after receiving tokens. This creates attack vectors that require extra caution in designing modules. Any combination of modules may lead to a possible exploit. To elaborate on the dangerousness of the re-entrancy attack, a possible scenario is presented.
Before the exploit, we first elaborate on three attack vectors:
- DebtIssuanceModule.sol#L131-L141 The issuance module would pull tokens from the sender before minting setToken.
Assume there are three compoenents in this set. 1. CDai. 2. wfCash In the _callTokensToSend, the setToken has received cdai and the totalSupply is still the same.
nonReentrantdoes not protect cross-contract re-entrancy. This means, that during theissueof issuance module, users can trigger other modules’ functions.- Restricted functions with
onlyManagerAndValidSetmodifier may be triggered by the exploiter as well. Manager of a setToken is usually a manager contract. Assume it’s a multisig-wallet, the exploiter can front-run the execute transaction and replay the payload during his exploit. Note, a private transaction from flash-bot can still be front-run. Please refer to the uncle bandit risk
Given the above attack vectors, the exploiter have enough weapons to exploit the setToken at a propriate time. Note that different combination of modules may have different exploit paths. As long as the above attack vectors remain, the setToken is vulnerable.
Assume a setToken with CompoundLeverageModule, NotionalTradeModule and BasicIssuanceModule with the following positions: 1. CDAI: 100 2. wfCash-DAI 100 and totalSupply = 100. The community decides to remove the compoundLeverageModule from the set token. Since notionalTradeModule can handle cDAI, the community vote to just call removeModule to remove compoundLeverageModule. The exploiter has the time to build an exploit and wait the right timing to come.
- The exploiter listen the manager multisig wallet.
- Exploiter issue 10 setToken.
- During the
_callTokensToSendofwfcash, the totalSupply = 100, CDAI = 110, wfCash-DAI = 110. - Call
syncofCompoundLeverageModule._getCollateralPositionget_cToken.balanceOf(address(_setToken)) = 110andtotalSupply = 100and update theDefaultUnitofCETH1,1X. - Replay multisig wallet’s payload and remove
compoundLeverageModule. - The
setTokencan no longer issue / redeem as it would raiseundercollateralizederror. Further,setValuerwould give a pumped valuation that may cause harm to other protocols.
Proof of Concept
POC
The exploit is quite lengthy. Please check the Attack.sol for the main exploit logic.
function register() public {
_ERC1820_REGISTRY.setInterfaceImplementer(address(this), _TOKENS_SENDER_INTERFACE_HASH, address(this));
_ERC1820_REGISTRY.setInterfaceImplementer(address(this), _TOKENS_SENDER_INTERFACE_HASH, address(this));
}
function attack(uint256 _amount) external {
cToken.approve(address(issueModule), uint256(-1));
wfCash.approve(address(issueModule), uint256(-1));
issueModule.issue(setToken, _amount, address(this));
}
function tokensToSend(
address operator,
address from,
address to,
uint256 amount,
bytes calldata userData,
bytes calldata operatorData
) external {
compoundModule.sync(setToken, false);
manager.removeModule(address(setToken));
}Recommended Mitigation Steps
The design choice of wfcash being an ERC777 seems unnecessary to me. Over the past two years, ERC777 leads to so many exploits. IMBTC-UNISWAP CREAM-AMP I recommend the team use ERC20 instead.
If the SetToken team considers supporting ERC777 necessary, I recommend implementing protocol-wide cross-contract reentrancy prevention. Please refer to Rari-Capital. Comptroller.sol#L1978-L2002
Note that, Rari was exploited given this reentrancy prevention. Simply making nonReentrant cross-contact prevention may not be enough. I recommend to setToken protocol going through every module and re-consider whether it’s re-entrancy safe.
ckoopmann (Index Coop) commented:
@jeffywu : What do you think ? Can we drop the ERC777 interface from wfCash (it’s not used by the NotionalTradeModule afaik). If not, I’ll have to review this issue in more details and see if we need a mitigation on our side.
Note that the issue mostly references the DebtIssuanceModule which we probably wont / cant change unless there is a major vulnerability.
jeffywu (Notional) confirmed and commented:
@ckoopmann I think we can just drop ERC777
[M-09] Silent overflow of _fCashAmount
Submitted by GreyArt, also found by Meera
Description
If a _fCashAmount value that is greater than uint88 is passed into the _mint function, downcasting it to uint88 will silently overflow.
Recommended Mitigation Steps
// Use a safe downcast function e.g. wfCashLogic::_safeUint88
function _safeUint88(uint256 x) internal pure returns (uint88) {hil
require(x <= uint256(type(uint88).max));
return uint88(x);
}ckoopmann (Index Coop) confirmed and commented:
This seems reasonable and we’ll probably adopt the suggested mitigation approach.
[M-10] User can alter amount returned by redeem function due to control transfer
Submitted by 0xDjango
Control is transferred to the receiver when receiving the ERC777. They are able to transfer the ERC777 to another account, at which time the before and after balance calculation will be incorrect.
uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);
if (msg.sender != owner) {
_spendAllowance(owner, msg.sender, shares);
}
_redeemInternal(shares, receiver, owner);
/////////////
Control is transferred to user. They can alter their balance here.
///////////
uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);
uint256 assets = balanceAfter - balanceBefore;
//////////
Assets can be as low as 0 if they have transferred the same amount out as received.
//////////
emit Withdraw(msg.sender, receiver, owner, assets, shares);
return assets;jeffywu (Notional) disputed and commented:
Control of the contract is not transferred to anyone during a
balanceOfcall which is read only. No state can be modified.
fatherGoose1 (warden) commented:
The control is transferred in
_redeemInternal()which calls_burn()on the ERC777 which transfers control.
_burn()function logic here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/109778c17c7020618ea4e035efb9f0f9b82d43ca/contracts/token/ERC777/ERC777.sol#L390-L400
jeffywu (Notional) confirmed and commented:
Understood, will change to confirmed.
Low Risk and Non-Critical Issues
For this contest, 61 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by berndartmueller received the top score from the judge.
The following wardens also submitted reports: IllIllI, GimelSec, sorrynotsorry, xiaoming90, GreyArt, Meera, jonah1005, joestakey, unforgiven, Funen, 0xf15ers, Ruhum, Picodes, hyh, fatherOfBlocks, Deivitto, zzzitron, Trumpero, TomJ, TerrierLover, sseefried, simon135, PierrickGT, oyc_109, minhquanym, ellahi, csanuragjain, cloudjunky, Chom, cccz, Bronicle, antonttc, 0xDjango, 0x29A, 0x1f8b, Sm4rty, saian, sach1r0, Nethermind, kenzo, hake, dipp, delfin454000, cryptphi, Cityscape, catchup, c3phas, 0xNineDec, 0xNazgul, 0xmint, _Adam, z3s, Waze, Tadashi, slywaters, Lambda, JC, hansfriese, ayeslick, and 0xkatana.
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:
test_wrapped_fcash.test_transfer_fcash_contractis skipped- many tests are failing on the forked mainnet due to using addresses with insufficient token balances (e.g.
whales.DAI_EOAhas insufficient DAI tokens) - the
notionalTradeModule.spectest suite has many openTODOs
Table of Contents
-
Low Risk
- [L-01] Zero-address checks are missing
- [L-02] Use of floating pragma
- [L-03] Events not emitted for important state changes
- [L-04] Matured fCash positions not automatically redeemed in
NotionalTradeModule.initialize - [L-05] Misleading
NotionalTradeModule._mintFCashPositionfunction comments] - [L-06] Misleading comment in
wfCashLogic._burnfunction - [L-07] Matured fCash can still be wrapped via
ERC1155transfer - [L-08] Contracts are using outdated OpenZeppelin version
^3.4.2-solc-0.7 - [L-09]
wfCashERC4626contract does not conform toEIP4626
-
Non-Critical Findings
- [N-01] Use the
isETHreturn value fromwfCashBase.getTokeninstead of checking equality withETH_ADDRESS
- [N-01] Use the
[L-01] Zero-address checks are missing
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
L140 - wrappedfCashFactory = _wrappedfCashFactory;
L141 - weth = _weth;
L30 - NotionalV2 = _notional;
L31 - WETH = _weth;
L18 - BEACON = _beacon;
Recommended Mitigation Steps
Add zero-address checks, e.g.:
require(_weth != address(0), "Zero-address");[L-02] Use of floating pragma
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
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
The function comments imply that matured fCash positions are redeemed within NotionalTradeModule.initialize. However, this redemption is not implemented in this function.
Findings
* 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
The function comments imply that the given fCash position is redeemed. However, this function implements minting fCash tokens.
Findings
* @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
The comment next to the _withdrawCashToAccount function call implies that the from address is the withdrawal receiver. However, opts.receiver is the receiver.
Findings
// 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
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
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
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:
- https://github.com/code-423n4/2022-02-hubble-findings/issues/81
- https://github.com/code-423n4/2022-02-hubble-findings/issues/81
Findings
Recommended Mitigation Steps
Consider using the latest OZ packages within package.json.
[L-09] wfCashERC4626 contract does not conform to EIP4626
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.
[N-01] Use the isETH return value from wfCashBase.getToken instead of checking equality with ETH_ADDRESS
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;
}ckoopmann (Index Coop) commented:
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.
Gas Optimizations
For this contest, 55 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: antonttc, Meera, 0xKitsune, joestakey, 0xkatana, 0x29A, 0x1f8b, Waze, PierrickGT, hyh, GreyArt, delfin454000, Chom, berndartmueller, TomJ, Tomio, TerrierLover, slywaters, simon135, rfa, 8olidity, 0xf15ers, _Adam, 0xSolus, UnusualTurtle, saian, sach1r0, Picodes, oyc_109, ellahi, Deivitto, catchup, c3phas, asutorufos, Sm4rty, samruna, kaden, Funen, fatherOfBlocks, ElKu, DavidGialdi, csanuragjain, Cityscape, 0xNazgul, 0v3rf10w, ynnad, Tadashi, minhquanym, Lambda, hansfriese, hake, Fitraldys, djxploit, and 0xmint.
| Issue | Instances | |
|---|---|---|
| 1 | Avoid contract existence checks by using solidity version 0.8.10 or later | 6 |
| 2 | Multiple accesses of a mapping/array should use a local variable cache | 5 |
| 3 | internal functions only called once can be inlined to save gas |
7 |
| 4 | <array>.length should not be looked up in every loop of a for-loop |
2 |
| 5 | require()/revert() strings longer than 32 bytes cost extra gas |
4 |
| 6 | private functions not called by the contract should be removed to save deployment gas |
1 |
| 7 | Optimize names to save gas | 1 |
| 8 | Using bools for storage incurs overhead |
3 |
| 9 | Use a more recent version of solidity | 1 |
| 10 | Use a more recent version of solidity | 1 |
| 11 | It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied |
7 |
| 12 | ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too) |
5 |
| 13 | Splitting require() statements that use && saves gas |
2 |
| 14 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead |
53 |
| 15 | abi.encode() is less efficient than abi.encodePacked() |
3 |
| 16 | Using private rather than public for constants, saves gas |
1 |
| 17 | Use custom errors rather than revert()/require() strings to save gas |
17 |
| 18 | Functions guaranteed to revert when called by normal users can be marked payable |
14 |
Total: 133 instances over 18 issues
[1] Avoid contract existence checks by using solidity version 0.8.10 or later
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
There are 6 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
/// @audit registerToIssuanceModule()
239: try IDebtIssuanceModule(modules[i]).registerToIssuanceModule(_setToken) {} catch {}
/// @audit unregisterFromIssuanceModule()
256: try IDebtIssuanceModule(modules[i]).unregisterFromIssuanceModule(setToken) {} catch {}
/// @audit allowance()
501: if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
/// @audit getDecodedID()
639: try IWrappedfCash(_fCashPosition).getDecodedID() returns(uint16 _currencyId, uint40 _maturity){File: notional-wrapped-fcash/contracts/wfCashERC4626.sol
/// @audit balanceOf()
212: uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);
/// @audit balanceOf()
219: uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);[2] Multiple accesses of a mapping/array should use a local variable cache
The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping’s value in a local storage variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations. Caching an array’s struct avoids recalculating the array offsets into memory
There are 5 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
/// @audit positions[i] on line 395
396: address component = positions[i].component;
/// @audit positions[i] on line 607
608: address component = positions[i].component;
/// @audit positions[i] on line 619
620: address component = positions[i].component;File: notional-wrapped-fcash/contracts/wfCashLogic.sol
/// @audit assets[0] on line 133
134: assets[0].maturity,
/// @audit assets[0] on line 134
135: assets[0].assetType[3] internal functions only called once can be inlined to save gas
Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 7 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
368: function _deployWrappedfCash(uint16 _currencyId, uint40 _maturity) internal returns(IWrappedfCashComplete) {
376: function _getWrappedfCash(uint16 _currencyId, uint40 _maturity) internal view returns(IWrappedfCashComplete) {
418 function _mintFCashPosition(
419 ISetToken _setToken,
420 IWrappedfCashComplete _fCashPosition,
421 IERC20 _sendToken,
422 uint256 _fCashAmount,
423 uint256 _maxSendAmount
424 )
425 internal
426: returns(uint256 sentAmount)
493 function _approve(
494 ISetToken _setToken,
495 IWrappedfCashComplete _fCashPosition,
496 IERC20 _sendToken,
497: uint256 _maxAssetAmount
537 function _redeem(
538 ISetToken _setToken,
539 IWrappedfCashComplete _fCashPosition,
540 uint256 _fCashAmount,
541: bool _toUnderlying
581 function _getUnderlyingAndAssetTokens(IWrappedfCashComplete _fCashPosition)
582 internal
583 view
584: returns(IERC20 underlyingToken, IERC20 assetToken)
596 function _getFCashPositions(ISetToken _setToken)
597 internal
598 view
599: returns(address[] memory fCashPositions)[4] <array>.length should not be looked up in every loop of a for-loop
The overheads outlined below are PER LOOP, excluding the first loop
- storage arrays incur a Gwarmaccess (100 gas)
- memory arrays use
MLOAD(3 gas) - calldata arrays use
CALLDATALOAD(3 gas)
Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 2 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #1
238: for(uint256 i = 0; i < modules.length; i++) {File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #2
254: for(uint256 i = 0; i < modules.length; i++) {[5] require()/revert() strings longer than 32 bytes cost extra gas
Each extra chunk of byetes past the original 32 iincurs an MSTORE which costs 3 gas
There are 4 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #1
169: require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #2
199: require(_setToken.isComponent(address(wrappedfCash)), "FCash to redeem must be an index component");File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #3
378: require(wrappedfCashAddress.isContract(), "WrappedfCash not deployed for given parameters");File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #4
573: require(_paymentToken == assetToken, "Token is neither asset nor underlying token");[6] private functions not called by the contract should be removed to save deployment gas
There is 1 instance of this issue:
File: notional-wrapped-fcash/contracts/wfCashERC4626.sol #1
243: function _safeNegInt88(uint256 x) private pure returns (int88) {[7] Optimize names to save gas
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There is 1 instance of this issue:
File: notional-wrapped-fcash/contracts/wfCashBase.sol #1
/// @audit getToken()
16: abstract contract wfCashBase is ERC777Upgradeable, IWrappedfCash {[8] Using bools for storage incurs overhead
// Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past
There are 3 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #1
112: mapping(ISetToken => bool) public redeemToUnderlying;File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #2
115: mapping(ISetToken => bool) public allowedSetTokens;File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #3
118: bool public anySetAllowed;[9] Use a more recent version of solidity
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There is 1 instance of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol #1
19: pragma solidity 0.6.10;[10] Use a more recent version of solidity
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
There is 1 instance of this issue:
File: notional-wrapped-fcash/contracts/wfCashERC4626.sol #1
2: pragma solidity ^0.8.0;[11] It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied
There are 7 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
48: address internal constant ETH_ADDRESS = address(0);
238: for(uint256 i = 0; i < modules.length; i++) {
254: for(uint256 i = 0; i < modules.length; i++) {
393: for(uint256 i = 0; i < positionsLength; i++) {
519: uint32 minImpliedRate = 0;
605: for(uint256 i = 0; i < positionsLength; i++) {
618: for(uint256 i = 0; i < positionsLength; i++) {[12] ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)
Saves 6 gas per loop
There are 5 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
238: for(uint256 i = 0; i < modules.length; i++) {
254: for(uint256 i = 0; i < modules.length; i++) {
393: for(uint256 i = 0; i < positionsLength; i++) {
605: for(uint256 i = 0; i < positionsLength; i++) {
618: for(uint256 i = 0; i < positionsLength; i++) {[13] Splitting require() statements that use && saves gas
See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper
There are 2 instances of this issue:
File: notional-wrapped-fcash/contracts/wfCashLogic.sol #1
116 require(
117 msg.sender == address(NotionalV2) &&
118 // Only accept the fcash id that corresponds to the listed currency and maturity
119 _id == fCashID &&
120 // Protect against signed value underflows
121 int256(_value) > 0,
122 "Invalid"
123: );File: notional-wrapped-fcash/contracts/wfCashLogic.sol #2
129 require(
130 ac.hasDebt == 0x00 &&
131 assets.length == 1 &&
132 EncodeDecode.encodeERC1155Id(
133 assets[0].currencyId,
134 assets[0].maturity,
135 assets[0].assetType
136 ) == fCashID
137: );[14] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
There are 53 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
158: uint16 _currencyId,
159: uint40 _maturity,
187: uint16 _currencyId,
188: uint40 _maturity,
368: function _deployWrappedfCash(uint16 _currencyId, uint40 _maturity) internal returns(IWrappedfCashComplete) {
368: function _deployWrappedfCash(uint16 _currencyId, uint40 _maturity) internal returns(IWrappedfCashComplete) {
376: function _getWrappedfCash(uint16 _currencyId, uint40 _maturity) internal view returns(IWrappedfCashComplete) {
376: function _getWrappedfCash(uint16 _currencyId, uint40 _maturity) internal view returns(IWrappedfCashComplete) {
519: uint32 minImpliedRate = 0;
545: uint32 maxImpliedRate = type(uint32).max;
639: try IWrappedfCash(_fCashPosition).getDecodedID() returns(uint16 _currencyId, uint40 _maturity){
639: try IWrappedfCash(_fCashPosition).getDecodedID() returns(uint16 _currencyId, uint40 _maturity){File: notional-wrapped-fcash/contracts/wfCashBase.sol
35: function initialize(uint16 currencyId, uint40 maturity) external override initializer {
35: function initialize(uint16 currencyId, uint40 maturity) external override initializer {
83: function getMaturity() public view override returns (uint40 maturity) {
93: function getCurrencyId() public view override returns (uint16 currencyId) {
98: function getDecodedID() public view override returns (uint16 currencyId, uint40 maturity) {
98: function getDecodedID() public view override returns (uint16 currencyId, uint40 maturity) {
103: function decimals() public pure override returns (uint8) {
109: function getMarketIndex() public view override returns (uint8) {File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol
15: event WrapperDeployed(uint16 currencyId, uint40 maturity, address wrapper);
15: event WrapperDeployed(uint16 currencyId, uint40 maturity, address wrapper);
21: function _getByteCode(uint16 currencyId, uint40 maturity) internal view returns (bytes memory) {
21: function _getByteCode(uint16 currencyId, uint40 maturity) internal view returns (bytes memory) {
26: function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
26: function deployWrapper(uint16 currencyId, uint40 maturity) external returns (address) {
39: function computeAddress(uint16 currencyId, uint40 maturity) public view returns (address) {
39: function computeAddress(uint16 currencyId, uint40 maturity) public view returns (address) {File: notional-wrapped-fcash/contracts/wfCashERC4626.sol
18: uint16 currencyId = getCurrencyId();
31: (uint16 currencyId, uint40 maturity) = getDecodedID();
31: (uint16 currencyId, uint40 maturity) = getDecodedID();
100: (uint16 currencyId, uint40 maturity) = getDecodedID();
100: (uint16 currencyId, uint40 maturity) = getDecodedID();
120: (uint16 currencyId, uint40 maturity) = getDecodedID();
120: (uint16 currencyId, uint40 maturity) = getDecodedID();
139: (uint16 currencyId, uint40 maturity) = getDecodedID();
139: (uint16 currencyId, uint40 maturity) = getDecodedID();
157: (uint16 currencyId, uint40 maturity) = getDecodedID();
157: (uint16 currencyId, uint40 maturity) = getDecodedID();
243: function _safeNegInt88(uint256 x) private pure returns (int88) {File: notional-wrapped-fcash/contracts/wfCashLogic.sol
29: uint88 fCashAmount,
31: uint32 minImpliedRate
43: uint88 fCashAmount,
45: uint32 minImpliedRate
52: uint88 fCashAmount,
54: uint32 minImpliedRate,
169: uint32 maxImpliedRate
187: uint32 maxImpliedRate
222: uint16 currencyId = getCurrencyId();
261: uint16 currencyId,
263: uint88 assetInternalCashClaim,
279: uint32 maxImpliedRate
315: function _safeUint88(uint256 x) internal pure returns (uint88) {[15] abi.encode() is less efficient than abi.encodePacked()
There are 3 instances of this issue:
File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol #1
23: return abi.encodePacked(type(nBeaconProxy).creationCode, abi.encode(BEACON, initCallData));File: notional-wrapped-fcash/contracts/wfCashERC4626.sol #2
230 bytes memory userData = abi.encode(
231 RedeemOpts({
232 redeemToUnderlying: true,
233 transferfCash: false,
234 receiver: receiver,
235 maxImpliedRate: 0
236 })
237: );File: notional-wrapped-fcash/contracts/wfCashLogic.sol #3
159: bytes memory data = abi.encode(opts);[16] Using private rather than public for constants, saves gas
If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table
There is 1 instance of this issue:
File: notional-wrapped-fcash/contracts/proxy/WrappedfCashFactory.sol #1
12: bytes32 public constant SALT = 0;[17] Use custom errors rather than revert()/require() strings to save gas
Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
There are 17 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
169: require(_setToken.isComponent(address(_sendToken)), "Send token must be an index component");
199: require(_setToken.isComponent(address(wrappedfCash)), "FCash to redeem must be an index component");
227: require(allowedSetTokens[_setToken], "Not allowed SetToken");
234: require(_setToken.isInitializedModule(getAndValidateAdapter(DEFAULT_ISSUANCE_MODULE_NAME)), "Issuance not initialized");
269: require(_setToken.isInitializedModule(address(_debtIssuanceModule)), "Issuance not initialized");
280: require(controller.isSet(address(_setToken)) || allowedSetTokens[_setToken], "Invalid SetToken");
378: require(wrappedfCashAddress.isContract(), "WrappedfCash not deployed for given parameters");
449: require(sentAmount <= _maxSendAmount, "Overspent");
485: require(receivedAmount >= _minReceiveAmount, "Not enough received amount");
573: require(_paymentToken == assetToken, "Token is neither asset nor underlying token");File: notional-wrapped-fcash/contracts/wfCashBase.sol
37: require(cashGroup.maxMarketIndex > 0, "Invalid currency");
40 require(
41 DateTime.isValidMaturity(cashGroup.maxMarketIndex, maturity, block.timestamp),
42 "Invalid maturity"
43: );File: notional-wrapped-fcash/contracts/wfCashERC4626.sol
23: require(underlyingExternal > 0, "Must Settle");File: notional-wrapped-fcash/contracts/wfCashLogic.sol
57: require(!hasMatured(), "fCash matured");
116 require(
117 msg.sender == address(NotionalV2) &&
118 // Only accept the fcash id that corresponds to the listed currency and maturity
119 _id == fCashID &&
120 // Protect against signed value underflows
121 int256(_value) > 0,
122 "Invalid"
123: );
211: require(opts.receiver != address(0), "Receiver is zero address");
225: require(0 < cashBalance, "Negative Cash Balance");[18] Functions guaranteed to revert when called by normal users can be marked payable
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
There are 14 instances of this issue:
File: index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol
156 function mintFCashPosition(
157 ISetToken _setToken,
158 uint16 _currencyId,
159 uint40 _maturity,
160 uint256 _mintAmount,
161 address _sendToken,
162 uint256 _maxSendAmount
163 )
164 external
165 nonReentrant
166 onlyManagerAndValidSet(_setToken)
167: returns(uint256)
185 function redeemFCashPosition(
186 ISetToken _setToken,
187 uint16 _currencyId,
188 uint40 _maturity,
189 uint256 _redeemAmount,
190 address _receiveToken,
191 uint256 _minReceiveAmount
192 )
193 external
194 nonReentrant
195 onlyManagerAndValidSet(_setToken)
196: returns(uint256)
210: function redeemMaturedPositions(ISetToken _setToken) public nonReentrant onlyValidAndInitializedSet(_setToken) {
219 function initialize(
220 ISetToken _setToken
221 )
222 external
223 onlySetManager(_setToken, msg.sender)
224: onlyValidAndPendingSet(_setToken)
219 function initialize(
220 ISetToken _setToken
221 )
222 external
223 onlySetManager(_setToken, msg.sender)
224: onlyValidAndPendingSet(_setToken)
246: function removeModule() external override onlyValidAndInitializedSet(ISetToken(msg.sender)) {
268: function registerToModule(ISetToken _setToken, IDebtIssuanceModule _debtIssuanceModule) external onlyManagerAndValidSet(_setToken) {
279: function updateAllowedSetToken(ISetToken _setToken, bool _status) external onlyOwner {
289: function updateAnySetAllowed(bool _anySetAllowed) external onlyOwner {
294 function setRedeemToUnderlying(
295 ISetToken _setToken,
296 bool _toUnderlying
297 )
298 external
299: onlyManagerAndValidSet(_setToken)
309: function moduleIssueHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
317: function moduleRedeemHook(ISetToken _setToken, uint256 /* _setTokenAmount */) external override onlyModule(_setToken) {
326 function componentIssueHook(
327 ISetToken _setToken,
328 uint256 _setTokenAmount,
329 IERC20 _component,
330 bool _isEquity
331: ) external override onlyModule(_setToken) {
338 function componentRedeemHook(
339 ISetToken _setToken,
340 uint256 _setTokenAmount,
341 IERC20 _component,
342 bool _isEquity
343: ) external override onlyModule(_setToken) {Well organized report
Disclosures
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.