robrobbins
commented
2 years ago coverage numbers mean little and are easily gamed
Open code423n4 opened 2 years ago
robrobbins
commented
2 years ago coverage numbers mean little and are easily gamed
L: OPEN TODO'S
Currently the
Swivel.solcontract has 17 open TODO comments pointing to non implemented features, comments or explanations. It is highly advisable to check this before going live. Also, it is remarked that any breaking changes introduced by the implementations of the comments won't be part of this audit so they might need further reviews to check their security.L: NO ACCESSIBLE COVERAGE REPORT
Besides from having a link within the project repo to get the test suite written mainly in Go, there is no documentation nor instructions about how coverage tests can be performed. It is advised having at least a 95% of contract testing coverage before deploying them to mainnet. Consider adding and making a coverage report accessible and document clearly how it can be ran.
L: KEY ACTIONS PERFORMED BY THE ADMIN/SWIVEL SHOULD EMIT EVENTS
The following functions do not emit events after they are called:
Swivel.setAdmin()Swivel.withdraw()Swivel.approveUnderlying()Marketplace.setAdmin()Marketplace.mintZcTokenAddingNotional()Marketplace.burnZcTokenRemovingNotional()Marketplace.transferVaultNotionalFee()Marketplace.pause()Among the functions mentioned before, there are a few events that are missing which are more important to emit than others. For example, the lack of emitted events while changing ownership or withdrawing ERC20 tokens from Swivel, as well as pausing the marketplace or transferring Notional fees are key actions that cannot pass unattended.
It is advisable to emit the respective events one the mentioned functions, having relevant indexed parameters on each event to allow offchain filtering.