QA Report #163
Labels
bug
Something isn't working
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Table Of Content
* Use safeApprove() instead approve()
* Unused success return value
* Use safeTransfer() instead transfer()
* Missing two steps verification process
* Wrong use of assert
* Avoid floating pragma
* Missing 0 address check at transfer
* Array access is out of bounds
* Should approve(0) first
* Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.
* Consider adding constant variables instead of hardcoded strings
* Add event to the following functions
* Events not emitted for important state changes
QA REPORT
Use safeApprove() instead approve()
Use openzeppelin safeApprove() method instead of approve() in the following locations.
Code Instances:
Unused success return value
The following calls ignores the return value of the called function that might indicate the the call failed.
Code Instances:
Use safeTransfer() instead transfer()
Use openzeppelin safeTransfer() method instead of transfer() in the following locations.
Code Instances:
Missing two steps verification process
The process of transferring ownership is dangerous since typing the wrong address can lead to severe implications. It is better to have to steps verification process with set and claim functions to decrease the chances of human error. Consider changing to two steps verification process of transferring privileges. Human mistakes can happen.
For instance, Kernel.t.sol
Wrong use of assert
You should use if-revert or require statements instead of assertions in production.
Code Instances:
Avoid floating pragma
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. (SWC-103)
Code Instances:
Missing 0 address check at transfer
Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.
Code Instances:
Array access is out of bounds
There is no check for the access to be in the array bounds.
Code Instances:
Should approve(0) first
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.
Code Instances:
Several functions are declaring named returns but then are using return statements. I suggest choosing only one for readability reasons.
Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.
Code Instances:
Consider adding constant variables instead of hardcoded strings
A good practice is to use constant variables instead of hardcoded strings in the code.
Code Instances:
Add event to the following functions
Code Instances:
Events not emitted for important state changes
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
Code Instances:
The text was updated successfully, but these errors were encountered: