Notional x Index Coop - xiaoming90's results

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L52 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/notional-wrapped-fcash/contracts/wfCashERC4626.sol#L134

Vulnerability details

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.

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

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.

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

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:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol

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.

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L309 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L385

Vulnerability details

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.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L309

/**
 * @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".

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L385

/**
 * @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 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 _redeemMaturedPositions really 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 _redeemMaturedPositions won'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.

Lines of code

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L418 https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L493

Vulnerability details

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.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L418

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.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L493

/**
 * @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 another party. Thus, there should not be any residual allowance that allows Notional's contract to withdraw fund 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);
-    }
}

Mismatch In The Number Of Parameters

The wfCashLogic.mintViaUnderlying and wfCashLogic.mintViaAsset functions only accept four (4) parameters.

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

function mintViaUnderlying(
    uint256 depositAmountExternal,
    uint88 fCashAmount,
    address receiver,
    uint32 minImpliedRate
) external override

function mintViaAsset(
    uint256 depositAmountExternal,
    uint88 fCashAmount,
    address receiver,
    uint32 minImpliedRate
) external override

However, within the NotionalTradeModule._mint, it attempts to call the wfCashLogic.mintViaUnderlying and wfCashLogic.mintViaAsset functions with five (5) parameters. For consistency, remove the last parameter from the ABI encoding as shown below:

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L510

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);
}

Lack of Input Validation

For defence-in-depth purpose, it is recommended to perform additional validation against the amount that the user is attempting to deposit, mint, withdraw and redeem to ensure that the submitted amount is valid.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7c75b8aa89073376fb67d78a40f6d69331092c94/contracts/token/ERC20/extensions/ERC20TokenizedVault.sol#L94

function deposit(uint256 assets, address receiver) public override returns (uint256) {
+		require(assets <= maxDeposit(receiver), "deposit more than max");
    uint256 shares = previewDeposit(assets);
    // Will revert if matured
    _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
    emit Deposit(msg.sender, receiver, assets, shares);
    return shares;
}

function mint(uint256 shares, address receiver) public override returns (uint256) {
+		require(shares <= maxMint(receiver), "mint more than max");
    uint256 assets = previewMint(shares);
    // Will revert if matured
    _mintInternal(assets, _safeUint88(shares), receiver, 0, true);
    emit Deposit(msg.sender, receiver, assets, shares);
    return assets;
}

function withdraw(
    uint256 assets,
    address receiver,
    address owner
) public override returns (uint256) {
+		require(assets <= maxWithdraw(owner), "withdraw more than max");
    uint256 shares = previewWithdraw(assets);

    if (msg.sender != owner) {
        _spendAllowance(owner, msg.sender, shares);
    }
    _redeemInternal(shares, receiver, owner);

    emit Withdraw(msg.sender, receiver, owner, assets, shares);

    return shares;
}

function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256) {
+    		require(shares <= maxRedeem(owner), "redeem more than max");
        // It is more accurate and gas efficient to check the balance of the
        // receiver here than rely on the previewRedeem method.
        uint256 balanceBefore = IERC20(asset()).balanceOf(receiver);

        if (msg.sender != owner) {
            _spendAllowance(owner, msg.sender, shares);
        }
        _redeemInternal(shares, receiver, owner);

        uint256 balanceAfter = IERC20(asset()).balanceOf(receiver);
        uint256 assets = balanceAfter - balanceBefore;
        emit Withdraw(msg.sender, receiver, owner, assets, shares);
        return assets;
    }

Approve Not Set To Zero First

Vulnerability Details

Not calling approve(0) first might cause the protocol to be vulnerable to Front-Run/Double-Spend Attack.

https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L493

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);
    }
}

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

function _approve(
    ISetToken _setToken,
    IWrappedfCashComplete _fCashPosition,
    IERC20 _sendToken,
    uint256 _maxAssetAmount
)
internal
{
    if(IERC20(_sendToken).allowance(address(_setToken), address(_fCashPosition)) < _maxAssetAmount) {
+       bytes memory approveZeroCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), 0);
+				_setToken.invoke(address(_sendToken), 0, approveZeroCallData);
        bytes memory approveCallData = abi.encodeWithSelector(_sendToken.approve.selector, address(_fCashPosition), _maxAssetAmount);
        _setToken.invoke(address(_sendToken), 0, approveCallData);
    }
}

Floating Pragma

Proof-of-Concept

The contract makes use of the floating-point pragma ^0.8.0. Contracts should be deployed using the same compiler version. Locking the pragma helps ensure that contracts are not unintentionally deployed using another pragma, such as an obsolete version, that may introduce issues in the contract system.

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

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./wfCashLogic.sol";
import "../interfaces/IERC4626.sol";

contract wfCashERC4626 is IERC4626, wfCashLogic {
    constructor(INotionalV2 _notional, WETH9 _weth) wfCashLogic(_notional, _weth) {}
   	..SNIP..

Recommended Mitigation Steps

Consider locking the pragma version. It is advised that floating pragma should not be used in production

Unused Function

Proof-of-Concept

Following function was defined, but not used in any of the contracts.

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

function _safeNegInt88(uint256 x) private pure returns (int88) {
    int256 y = -int256(x);
    require(int256(type(int88).min) <= y);
    return int88(y);
}

Recommended Mitigation Steps

It is recommended to remove this function if it is not required.