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#L650-L680

Vulnerability details

Impact

Contract NotionalTradeModule.sol will not compile due to an error caused by missing import of safeMath and the directive using for.

Since safeMath is not imported and no using for directive, the contract would not even compile as it would throw an error on [line 677] https://github.com/code-423n4/2022-06-notional-coop/blob/6f8c325f604e2576e2fe257b6b57892ca181509a/index-coop-notional-trade-module/contracts/protocol/modules/v1/NotionalTradeModule.sol#L677

            preTradeSendTokenBalance.sub(currentSendTokenBalance),

The contract NotionalTradeModule.sol has a function called _updateSetTokenPositions which according to the comments was copied from the trade module here

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

It's important to note one of the comments in the function _updateSetTokenPositions from NotionalTradeModule.sol

* @dev WARNING: This function is largely copied from the trade module

In this function,ie _updateSetTokenPositions , the return statement executes an arithmetic operation using safeMath sub function. However while coping majority of this function from the trade module the safeMath library was never imported.

Note the return statement :

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

To use safeMath we often use the using for directive as follows

using safeMath for uint256;

But the above directive is also not present on the contract NotionalTradeModule.sol.

Proof of Concept

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

The NotionalTradeModule.sol contract only imports the following. imports

import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { IERC777 } from "@openzeppelin/contracts/token/ERC777/IERC777.sol";
import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
import { Address } from "@openzeppelin/contracts/utils/Address.sol";

import { IController } from "../../../interfaces/IController.sol";
import { IDebtIssuanceModule } from "../../../interfaces/IDebtIssuanceModule.sol";
import { IModuleIssuanceHook } from "../../../interfaces/IModuleIssuanceHook.sol";
import { IWrappedfCash, IWrappedfCashComplete } from "../../../interfaces/IWrappedFCash.sol";
import { IWrappedfCashFactory } from "../../../interfaces/IWrappedFCashFactory.sol";
import { ISetToken } from "../../../interfaces/ISetToken.sol";
import { ModuleBase } from "../../lib/ModuleBase.sol";

Available using for directive The only available using for directive is the address as shown below.

    using Address for address;

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

Tools Used

Vim,Remix

Recommended Mitigation Steps

Importing the safeMath library from openzeppelin and add the using for directive as follows using safeMath for uint256; should fix the issue

This bug is as a result of Copy and paste from other contracts therefore I would advice you to avoid copy pasting and if we must be careful when copying code from other contracts.

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