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

0 stars 1 forks source link

QA Report #95

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
Overview Risk Rating Number of issues
Low Risk 2
Non-Critical Risk 4

Table of Contents

1. Low Risk Issues

1.1. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

Creator/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
Creator/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
Creator/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
Creator/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
Creator/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {
Marketplace/Compounding.sol:51:    if (p == uint8(Protocols.Compound)) {
Marketplace/Compounding.sol:53:    } else if (p == uint8(Protocols.Rari)) {
Marketplace/Compounding.sol:55:    } else if (p == uint8(Protocols.Yearn)) {
Marketplace/Compounding.sol:57:    } else if (p == uint8(Protocols.Aave)) {
Marketplace/Compounding.sol:59:    } else if (p == uint8(Protocols.Euler)) {
Marketplace/Compounding.sol:69:    if (p == uint8(Protocols.Compound)) {
Marketplace/Compounding.sol:71:    } else if (p == uint8(Protocols.Rari)) { 
Marketplace/Compounding.sol:73:    } else if (p == uint8(Protocols.Yearn)) {
Marketplace/Compounding.sol:75:    } else if (p == uint8(Protocols.Aave)) {
Marketplace/Compounding.sol:78:    } else if (p == uint8(Protocols.Euler)) {
Swivel/Swivel.sol:708:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:710:    } else if (p == uint8(Protocols.Yearn)) {
Swivel/Swivel.sol:713:    } else if (p == uint8(Protocols.Aave)) {
Swivel/Swivel.sol:719:    } else if (p == uint8(Protocols.Euler)) {
Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:743:    } else if (p == uint8(Protocols.Yearn)) {
Swivel/Swivel.sol:746:    } else if (p == uint8(Protocols.Aave)) {
Swivel/Swivel.sol:750:    } else if (p == uint8(Protocols.Euler)) {
Tokens/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
Tokens/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
Tokens/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
Tokens/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
Tokens/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {
VaultTracker/Compounding.sol:41:    if (p == uint8(Protocols.Compound)) {
VaultTracker/Compounding.sol:43:    } else if (p == uint8(Protocols.Rari)) { 
VaultTracker/Compounding.sol:45:    } else if (p == uint8(Protocols.Yearn)) {
VaultTracker/Compounding.sol:47:    } else if (p == uint8(Protocols.Aave)) {
VaultTracker/Compounding.sol:50:    } else if (p == uint8(Protocols.Euler)) {

1.2. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

Creator/VaultTracker.sol:21:  address public immutable admin;
Creator/VaultTracker.sol:22:  address public immutable cTokenAddr;
Creator/VaultTracker.sol:23:  address public immutable swivel;
Creator/ZcToken.sol:15:    address public override immutable underlying;
Creator/ZcToken.sol:21:    address public immutable cToken;
Marketplace/MarketPlace.sol:26:  address public immutable creator;
Swivel/Swivel.sol:29:  address public immutable marketPlace;
Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
Tokens/ZcToken.sol:15:    address public override immutable underlying;
Tokens/ZcToken.sol:21:    address public immutable cToken;
VaultTracker/VaultTracker.sol:21:  address public immutable admin;
VaultTracker/VaultTracker.sol:22:  address public immutable cTokenAddr;
VaultTracker/VaultTracker.sol:23:  address public immutable swivel;

2. Non-Critical Issues

2.1. Typos

Creator/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.
Tokens/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.
Marketplace/Erc20.sol:2:// Inspired on token.sol from DappHub. Natspec adpated from OpenZeppelin.
Creator/ZcToken.sol:9:// Utilizing an external custody contract to allow for backwards compatability with some projects.
Creator/ZcToken.sol:22:    /// @dev address and interface for an external custody contract (necessary for some project's backwards compatability)
Tokens/ZcToken.sol:9:// Utilizing an external custody contract to allow for backwards compatability with some projects.
Tokens/ZcToken.sol:22:    /// @dev address and interface for an external custody contract (necessary for some project's backwards compatability)
Tokens/ZcToken.sol:67:    /// @notice Post maturity simulates the effects of redeemption at the current block. Returns 0 pre-maturity.
Creator/ZcToken.sol:67:    /// @notice Post maturity simulates the effects of redeemption at the current block. Returns 0 pre-maturity.
Creator/ZcToken.sol:145:    /// @param t Address recieving the minted amount
Tokens/ZcToken.sol:145:    /// @param t Address recieving the minted amount
Marketplace/Compounding.sol:34:  // @dev Deployed ddress of the associated Aave Pool
Swivel/Swivel.sol:677:    // NOTE: for swivel reddem there is no transfer out as there is in redeemVaultInterest
Swivel/Swivel.sol:682:  /// @notice Varifies the validity of an order and it's signature.
Swivel/Swivel.sol:747:      // Aave v2 docs state that withraw returns uint256

2.2. Duplicated code: Hardcoded strings having multiple occurrences should be declared as constant

Creator/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
Creator/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
Tokens/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
Tokens/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
VaultTracker/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
VaultTracker/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");

2.3. Open TODOS

Consider resolving the TODOs before deploying.

Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
Swivel/Swivel.sol:120:    // TODO cheaper to assign amount here or keep the ADD?
Swivel/Swivel.sol:157:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:192:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:221:    // TODO assign amount or keep ADD?
Swivel/Swivel.sol:286:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:317:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:347:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:382:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:707:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
Swivel/Swivel.sol:708:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:716:      // TODO explain the Aave deposit args
Swivel/Swivel.sol:721:      // TODO explain the 0 (primary account)
Swivel/Swivel.sol:740:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:748:      // TODO explain the withdraw args
Swivel/Swivel.sol:752:      // TODO explain the 0

2.4. The pragmas used are not the same everywhere

Creator/Compounding.sol:3:pragma solidity 0.8.13;
Creator/Creator.sol:3:pragma solidity 0.8.13;
Creator/Erc20.sol:4:pragma solidity ^0.8.0;
Creator/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Creator/IERC5095.sol:2:pragma solidity ^0.8.0;
Creator/Interfaces.sol:3:pragma solidity 0.8.13;
Creator/IRedeemer.sol:2:pragma solidity ^0.8.0;
Creator/LibCompound.sol:2:pragma solidity >=0.8.4;
Creator/LibFuse.sol:1:pragma solidity 0.8.13;
Creator/Protocols.sol:3:pragma solidity 0.8.13;
Creator/VaultTracker.sol:3:pragma solidity 0.8.13;
Creator/ZcToken.sol:2:pragma solidity ^0.8.4;
Marketplace/Compounding.sol:3:pragma solidity 0.8.13;
Marketplace/Erc20.sol:4:pragma solidity ^0.8.0;
Marketplace/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Marketplace/Interfaces.sol:3:pragma solidity 0.8.13;
Marketplace/LibCompound.sol:2:pragma solidity >=0.8.4;
Marketplace/LibFuse.sol:1:pragma solidity 0.8.13;
Marketplace/MarketPlace.sol:3:pragma solidity 0.8.13;
Marketplace/Protocols.sol:3:pragma solidity 0.8.13;
Swivel/Hash.sol:3:pragma solidity 0.8.13;
Swivel/Interfaces.sol:3:pragma solidity 0.8.13;
Swivel/Protocols.sol:3:pragma solidity 0.8.13;
Swivel/Safe.sol:3:pragma solidity 0.8.13;
Swivel/Sig.sol:3:pragma solidity 0.8.13;
Swivel/Swivel.sol:3:pragma solidity 0.8.13;
Tokens/Compounding.sol:3:pragma solidity 0.8.13;
Tokens/Erc20.sol:4:pragma solidity ^0.8.0;
Tokens/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
Tokens/IERC5095.sol:2:pragma solidity ^0.8.0;
Tokens/Interfaces.sol:3:pragma solidity 0.8.13;
Tokens/IRedeemer.sol:2:pragma solidity ^0.8.0;
Tokens/LibCompound.sol:2:pragma solidity >=0.8.4;
Tokens/LibFuse.sol:1:pragma solidity 0.8.13;
Tokens/Protocols.sol:3:pragma solidity 0.8.13;
Tokens/ZcToken.sol:2:pragma solidity ^0.8.4;
VaultTracker/Compounding.sol:3:pragma solidity 0.8.13;
VaultTracker/FixedPointMathLib.sol:2:pragma solidity >=0.8.0;
VaultTracker/Interfaces.sol:3:pragma solidity 0.8.13;
VaultTracker/LibCompound.sol:2:pragma solidity >=0.8.4;
VaultTracker/LibFuse.sol:1:pragma solidity 0.8.13;
VaultTracker/Protocols.sol:3:pragma solidity 0.8.13;
VaultTracker/VaultTracker.sol:3:pragma solidity 0.8.13;
robrobbins commented 2 years ago

addressed elsewhere or non issue (ex the uint256 cast -- we control that uint8)