Platform: Code4rena
Start Date: 07/06/2022
Pot Size: $75,000 USDC
Total HM: 11
Participants: 77
Period: 7 days
Judge: gzeon
Total Solo HM: 7
Id: 124
League: ETH
Rank: 56/77
Findings: 1
Award: $89.19
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: berndartmueller
Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xkatana, 0xmint, Bronicle, Chom, Cityscape, Deivitto, Funen, GimelSec, GreyArt, IllIllI, JC, Lambda, Meera, Nethermind, Picodes, PierrickGT, Ruhum, Sm4rty, Tadashi, TerrierLover, TomJ, Trumpero, Waze, _Adam, antonttc, ayeslick, c3phas, catchup, cccz, cloudjunky, cryptphi, csanuragjain, delfin454000, dipp, ellahi, fatherOfBlocks, hake, hansfriese, hyh, joestakey, jonah1005, kenzo, minhquanym, oyc_109, sach1r0, saian, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, z3s, zzzitron
89.1872 USDC - $89.19
Certain outputs of some functions of the contracts in scope are unused, and comment blocks fill the position where the unused output variables lie.
Unused outputs from functions have been marked with comment blocks. This is not considered standard notation and may confuse readers. To illustrate what the issue is, let foo() be a function with 3 outputs (that are bools) and assume that outputs number 2 and 3 are unused:
(bool output1, /* */, /* */) = foo();
Remove the block comments, like in the following example
(bool output1, , ) = foo();
Stale comments (and old commented code) pollute the codebase by reducing readability of the file.
The presence of stale comments and commented code is dangerous, because as the code version improves, the commented code may become incompatible with the codebase, and uncommenting it may result in fatal errors.
The best practice would be to remove these stale comments. The best manner to handle old versions of the codebase would be through a version-control software.
There is a strict inequality used to perform non-negativity check.
The _burn function within wfCashLogic may revert when cashBalance is equal to zero. The revert message states "Negative Cash Balance", so this would be misleading.
Although the scenario where cashBalance == 0 is true may not occur, for the sake of coherency, the require statement should be changed either to
require(0 <= cashBalance, "Negative Cash Balance");
or to
require(0 < cashBalance, "Negative or Zero Cash Balance");
Return value of function is not assigned to any variable nor used.
It is bad practice to ignore altogether the returns of functions, because they might contain information about the execution (success or not) for example. In this particular case, there are several instances of calling the invoke on a SetToken instance, whose output is ignored.
invoke returns the bytes returned by a functionCallWithValue to approveCallData.
It might not be of interest whatsoever to check this value as long es the calldata is approved. Nevertheless, it raises a small concern.
Make sure that the return value is not needed whatsoever. It would be a good idea to add a comment explaining why the return value is unassigned.
On a local scope, local variables that share name with global variables, shadow these global variables (which become unaccessible within that scope).
The NotionalTradeModule contract inherits (among others) from ReentrancyGuard. Thus, it has a global variable called _status, which controls whether the contract has been already called by an external call (to avoid reentrancy attacks).
However, the function updateAllowedSetToken has one of its parameters named _status as well. So, within that function scope, the global _status is shadowed by the local _status.
This should not be of much concern, as the global _status variable is only important in the context of the nonReentrant modifier.
Either do nothing (because it is not important) or change the local variable name _status to something else.