Swivel contest - loop's results

Handle

loop

Vulnerability details

Line 124 of Swivel.sol describes the parameter uint256 a, but has wrong parameter name: /// @param o Amount of volume (principal) being filled by the taker's exit

Impact

No direct impact apart from code readability

Proof of Concept

Code snippet for function declaration + spec:

/// @notice Allows a user to initiate a zcToken by filling an offline vault initiate order
  /// @dev This method should pass (underlying, maturity, sender, maker, a) to MarketPlace.custodialInitiate
  /// @param o Order being filled
  /// @param o Amount of volume (principal) being filled by the taker's exit
  /// @param c Components of a valid ECDSA signature
  function initiateZcTokenFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal

Recommended Mitigation Steps

Change the second o to a

Handle

loop

Vulnerability details

The following functions are declared as public but can be external:

• createMarket (https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L53-L70)
• transferVaultNotional (https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L234-L238)
• transferVaultNotionalFee (https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L245-L248)
• balancesOf (https://github.com/Swivel-Finance/gost/blob/v2/test/vaulttracker/VaultTracker.sol#L243-L245)

Proof of Concept

Slither reference: https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-external

Tools Used

Slither

Handle

loop

Vulnerability details

uint16[] public fenominator; on line 24 of Swivel.sol is currently a dynamic size array. It does however only hold 4 values ([zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit]).

These are also the only values used in the initiate and exit functions where they use the respective fenominator at index 0-3 to calculate the fee.

Can be changed into a fixed array size with 4 elements to save a small amount of gas when contract gets deployed and array interacted with.

Recommended Steps

Change array declaration to: uint16[4] public fenominator;

Handle

loop

Vulnerability details

When calculating principalFilled or premiumFilled and fee amounts in the exit and initiate functions the 2 1e18 values cancel each other out.

Impact

Adds unnecessary operations to calculating the principal-premium-fee calculation.

Proof of Concept

When selling nTokens principalFilled is used:

uint256 principalFilled = (((a * 1e18) / o.premium) * o.principal) / 1e18;
uint256 fee = ((principalFilled * 1e18) / fenominator[x]) / 1e18;

When buying nTokens premiumFilled is used:

uint256 premiumFilled = (((a * 1e18) / o.principal) * o.premium) / 1e18;
uint256 fee = ((premiumFilled * 1e18) / fenominator[x]) / 1e18;

Note: x is the relevant fenominator used for the different initiate and exit functions.

Both calculations are however similar and in both cases the multiplication by 1e18 at the start and division by 1e18 at the end cancel each other out unless I missed an edge-case. When testing removing the 1e18's from the calculation always gave the same result at a lower gas price.

Lines where calculation is used:

• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L98-L99
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L133-L134
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L165-L166
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L188-L189
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L248-L249
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L275-L276
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L303-L304
• https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L336-L337

Tools Used

Remix

Handle

loop

Vulnerability details

On lines 17 & 18 the following string literals are declared: string constant public NAME = 'Swivel Finance'; string constant public VERSION = '2.0.0';

bytes32 constant can be used in stead of string constant to slightly reduce gas cost as neither of the two strings are larger than 32 bytes.

As a result NAME and VERSION can also be assigned in smaller bytes packing it together with HOLD like this:

bytes16 constant public NAME = 'Swivel Finance';
bytes8 constant public VERSION = '2.0.0';
uint64 constant public HOLD = 259200;

Proof of Concept

Used following contract code in remix to compare cost:

pragma solidity 0.8.4;

contract Test {
    string constant public NAME = 'Swivel Finance';
    string constant public VERSION = '2.0.0';
    
    bytes32 constant public NAME32 = 'Swivel Finance';
    bytes32 constant public VERSION32 = '2.0.0';
    uint256 constant public HOLD = 259200;
      
    bytes16 constant public NAME16 = 'Swivel Finance';
    bytes8 constant public VERSION8 = '2.0.0';
    uint64 constant public HOLD64 = 259200;
}

Tools Used

Remix

Handle

loop

Vulnerability details

the parameter uint8 d of the createMarket function is lacking a parameter description in function spec.

Impact

No direct impact, but with the parameter naming scheme of only using the first letter of its description the parameter spec is essential.

Proof of Concept

Code snippet for funciton spec + declaration:

/// @notice Allows the owner to create new markets
/// @param u Underlying token address associated with the new market
/// @param m Maturity timestamp of the new market
/// @param c cToken address associated with underlying for the new market
/// @param n Name of the new zcToken market
/// @param s Symbol of the new zcToken market
function createMarket(
    address u,
    uint256 m,
    address c,
    string memory n,
    string memory s,
    uint8 d
) public onlyAdmin(admin) returns (bool)

Recommended Mitigation Steps

Add parameter spec for uint8 d