Swivel v3 contest - 0xNazgul's results

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.

Use of 2**256 - 1 When 2**255 Should Be Used

Context: Swivel.sol#L549

Description: Infinity can also be represented via ``2255, it's hex representation is 0x8000000000000000000000000000000000000000000000000000000000000000while2256 - 1is0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff`. Then main difference is and where the gas savings come from is, zeros are cheaper than non-zero values in hex representation.

Recommendation: Use 2**255 instead of 2**256 - 1 to save gas on deployment.

Use of The memory Keyword When storage Should Be Used

Context: MarketPlace.sol#L91, MarketPlace.sol#L132, MarketPlace.sol#L216, MarketPlace.sol#L228, MarketPlace.sol#L248, MarketPlace.sol#L266, MarketPlace.sol#L284

Description: When copying a state struct in memory, there are as many SLOADs and MSTOREs as there are fields. When reading the whole struct multiple times is not needed, it's better to actually only read the relevant field(s). When only some of the fields are read several times, these particular values should be cached instead of the whole state struct.

Recommendation: Use the storage keyword instead of the memory keyword in these situations.

Same State Variable Read More Than Once

Context: VaultTracker.sol#L49-L77 (maturityRate), VaultTracker.sol#L82-L109 (maturityRate), VaultTracker.sol#L113-L139 (maturityRate), VaultTracker.sol#L152-L203 (maturityRate), VaultTracker.sol#L208-L239 (maturityRate), ZcToken.sol#L43-L48 (maturity, redeemer, protocol), ZcToken.sol#L52-L57 (maturity, redeemer, protocol), ZcToken.sol#L70-L75 (maturity, redeemer, protocol), ZcToken.sol#L79-L84 (maturity, redeemer, protocol), ZcToken.sol#L88-L93 (maturity, redeemer, protocol), ZcToken.sol#L98-L119 (maturity, redeemer, protocol), ZcToken.sol#L124-L137 (maturity, redeemer, protocol)

Description: Functions that read state variables more than once can cache it into a local variable for repeated reads saving gas by converting expensive SLOADs into much cheaper MLOADs.

Recommendation: It's best to cach the state variables into memory when read more than once.

State Variables That Can Be Set To Immutable

Context: Swivel.sol#L33

Description: Solidity 0.6.5 introduced immutable as a major feature. It allows setting contract-level variables at construction time which gets stored in code rather than storage. Each call to it reads from storage, using a sload costing 2100 gas cold or 100 gas warm. Setting it to immutable will have each storage read of the state variable to be replaced by the instruction push32 value, where value is set during contract construction time and this costs only 3 gas.

Recommendation: Set the state variable to immutable

Setting The Constructor To Payable

Context: All Contracts

Description: You can cut out 10 opcodes in the creation-time EVM bytecode if you declare a constructor payable. Making the constructor payable eliminates the need for an initial check of msg.value == 0 and saves 21 gas on deployment with no security risks.

Recommendation: Set the constructor to payable.

Function Ordering via Method ID

Context: All Contracts

Description: Contracts most called functions could simply save gas by function ordering via Method ID. Calling a function at runtime will be cheaper if the function is positioned earlier in the order (has a relatively lower Method ID) because 22 gas are added to the cost of a function for every position that came before it. The caller can save on gas if you prioritize most called functions. One could use This tool to help find alternative function names with lower Method IDs while keeping the original name intact.

Recommendation: Find a lower method ID name for the most called functions for example mostCalled() vs. mostCalled_41q() is cheaper by 44 gas.