search

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

1 stars 1 forks source link

QA Report #220

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Low Risk and Non-Critical Issues

1. Comment blocks on unused function outputs

Summary

Certain outputs of some functions of the contracts in scope are unused, and comment blocks fill the position where the unused output variables lie.

Details

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();

Github Permalinks

Mitigation

Remove the block comments, like in the following example

(bool output1, , ) = foo();

2. Presence of stale comments

Summary

Stale comments (and old commented code) pollute the codebase by reducing readability of the file.

Details

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.

Github Permalinks

Mitigation

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.

3. Strict inequality at balance check

Summary

There is a strict inequality used to perform non-negativity check.

Details

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.

Github Permalinks

Mitigation

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");

4. Ignored return value of function (Unknown Issue)

Summary

Return value of function is not assigned to any variable nor used.

Details

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.

Github Permalinks

Mitigation

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.

5. Global variable shadowed by local variable (Unknown issue)

Summary

On a local scope, local variables that share name with global variables, shadow these global variables (which become unaccessible within that scope).

Details

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.

Github Permalinks

Mitigation

Either do nothing (because it is not important) or change the local variable name _status to something else.