code423n4 commented 2 years ago

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#L490-L505

Vulnerability details

Impact

most of the tokens won't allow to call approve() function when the current allowance is not 0. for example openzeppelin is like this which is mostly used by protocols. in _approve() of NotionalTradeModule, contract don't first set approve value to 0 and tries to set it to _maxAssetAmount in one call. so if there were some dust left in that allowance that call will revert and whole transaction will fail. because _mintFCashPosition() uses _approve() so mintFCashPosition() functionality of contract won't work. There is high chance that could make dust in allowance. because in minting wfcash the exact amount of underlying token is clear and wfcash returns extra amount of underlying or asset token. so if in wfcash logic, code don't transfer the extra amount in the first place so that would make some dust to stay in allowance.

Proof of Concept

This is _approve() code in NotionalTradeModule:

    /**
     * @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);
        }
    }

As you can see the codes checks the current allowance of _setToken for _fCashPosition in _sendToken and if it was less than _maxAssetAmount, it make _setToken to calls approve() in _sendtoken and set the allowance to _maxAssetAmount. but if there were some dust left in allowance then this call will revert becasue in most of token implementations, the logic won't allow setting approval amount when it's not 0 and this will cause the whole transaction to revert. Because _mintFCashPosition() uses the _approve() function so mintFCashPosition() will be fail for that fcash and setToken and manager can't add that fcash to setToken portfolio.

The exact amount of underlying or asset token to mint _mintFCashPosition is determined by NotionalV2 and there are always some extra amount of underlying or asset remain in wfCash which send them back to the minter. so if wfCash don't transfer all the tokens in the first place and only transfer the quired ones then for sure some dust will remain in allowance. In general for many reason dust can remain in allowance, so it's safer if the contract don't take any chance and set the approval to 0x0 first then set it to _maxAssetAmount.

Tools Used

VIM

Recommended Mitigation Steps

set the approval to 0x0 first then set it to _maxAssetAmount.

ghost commented 2 years ago

I think there should never be an instance where there is dust in allowance.

When NotionalTradeModule._approve() is called, setToken (owner) gives _maxAssetAmount of allowance to the fCashPosition (spender) for _sendToken.

When NotionalTradeModule._mint() is called, either wfCashLogic.mintViaAsset or wfCashLogic.mintViaUnderlying functions is executed. These functions will do safeTransferFrom (located here a, b) where owner is the setToken, spender is the fCashPosition and amount is _maxAssetAmount

ckoopmann commented 2 years ago

I think this is a duplicate of: https://github.com/code-423n4/2022-06-notional-coop-findings/issues/221

gzeoneth commented 2 years ago

Consider with #80

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