code-423n4 / 2022-07-swivel-findings

0 stars 1 forks source link

QA Report #110

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Max/Infinite Approvals are Dangerous

Severity: Low Context: Swivel.sol#L544-L569

Description: Giving max/infinite approvals to contracts are dangerous because if those contracts are exploited then they can remove all the funds from the approving addresses.

Recommendation: Check allowance and approve as much as required.

Missing SPDX License Identifier (Out of Scope)

Severity: Informational Context: LibFuse.sol (All 3 contracts)

Description: These contracts are missing the SPDX license Identifier before the pragma version.

Recommendation: Consider adding the SPDX license, more specifically, // SPDX-License-Identifier: AGPL-3.0-only from transmissions11's original contract.

Unintuitive Modifier Name

Severity: Informational Context: ZcToken.sol#L152-L155 (onlyAdmin => onlyAuthorised),

Description: The modifier name is either not as it describes or can be more descriptive.

Recommendation: Change all occurrences of this modifier to be more intuitive.

TODOs Left In The Code

Severity: Informational Context: Swivel.sol#L33, Swivel.sol#L120, Swivel.sol#L157, Swivel.sol#L192, Swivel.sol#L221, Swivel.sol#L286, Swivel.sol#L317, Swivel.sol#L347, Swivel.sol#L382, Swivel.sol#L707, Swivel.sol#L708, Swivel.sol#L716, Swivel.sol#L721, Swivel.sol#L740, Swivel.sol#L741, Swivel.sol#L748, Swivel.sol#L752

Description: There should never be any TODOs in the code when deploying.

Recommendation: Finish the TODOs before deploying.

Spelling Errors

Severity: Informational Context: Swivel.sol#L677 (reddem => redeem), Swivel.sol#L682 (Varifies => Verifies), Swivel.sol#L747 (withraw => withdraw), ZcToken.sol#L9 (compatability => compatibility), ZcToken.sol#L22 (compatability => compatibility), ZcToken.sol#L127 (delete it 2nd can), ZcToken.sol#L145 (recieving => receiving)

Description: Spelling errors in comments can cause confusion to both users and developers.

Recommendation: Check all misspellings to ensure they are corrected.

Missing or Incomplete NatSpec

Severity: Informational Context: All Contracts

Description: Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Recommendation: Add in full NatSpec comments for all functions to have complete code documentation for future use.

Floating Pragma

Severity: Informational Context: ZcToken.sol

Description: 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.

Recommendation: Lock the pragma version.

Multiple Solidity Pragma

Severity: Informational Context: All Contracts

Description: It is better to use one Solidity compiler version across all contracts instead of different versions with different bugs and security checks.

Recommendation: Ensure all pragma versions are the same one.

Older Version Pragma

Severity: Informational Context: ZcToken.sol

Description: Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs.

Recommendation: Consider using the most recent version.