`IsWrappedFcash` check is a gas bomb

[code423n4]

Lines of code

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

Vulnerability details

Impact

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 exmple. 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).

Test.sol

    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 19788041

Tools Used

Manual inspection. 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]

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 getDecodedID call 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]

Valid but don't think this is High Risk, eth_estimateGas should fail preventing most user from interacting with a ridiculous gas limit.