Submitted by jonah1005
https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/modules/v1/DebtIssuanceModule.sol#L131-L141
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC777/ERC777.sol#L376-L380
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:
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.
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.
POC
The exploit is quite lengthy. Please check the Attack.sol for the main exploit logic.
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 its re-entrancy safe.
ckoopmann (Index Coop) commented:
jeffywu (Notional) confirmed and commented:
