Swivel v3 contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L112-L115 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L133-L134

Vulnerability details

Incorrect check in ZcToken.withdraw and ZcToken.redeem leads to underlying tokens not being able to be transferred

In both ZcToken.withdraw and ZcToken.redeem, in the case where holder != msg.sender, a check of the msg.sender's ZcToken allowance is performed. But the functions revert if the allowance is larger than the amount of tokens being redeemed. So the call only goes to the next step if allowance[holder][msg.sender] < principalAmount, which would then make the following line revert. In conclusion, the functions will always revert in this case, meaning an approved caller will never be able to redeem the desired amount of underlying tokens.

Impact

High

Mitigation

Change the allowance checks:

-112:             if (allowed >= previewAmount) {
+112:             if (allowed < previewAmount) {
113:                 revert Approvals(allowed, previewAmount);
114:             }

-133:             if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
+133:             if (allowed < principalAmount) { revert Approvals(allowed, principalAmount); } 

QA Report

Table of Contents

• summary

Low

• Contracts should inherit their interfaces
• Immutable addresses lack zero-address check
• Safe.sol does not check contract existence
• Setters should check the input value
• Wrong function name

Non-critical

• 2**256 - 1 can be re-written
• Constants instead of magic numbers
• Events indexing
• Event should be emitted in setters
• Function order
• Long lines
• Natspec
• Non-library files should use fixed compiler versions
• Non-library files should use the same compiler version
• open TODOs
• Public functions can be external
• Tautological code
• Typos

summary

The general concerns are with:

Contracts should inherit their interfaces

IMPACT

Contract implementations should inherit their interfaces. Extending an interface ensures that all function signatures are correct, and catches mistakes introduced (e.g. through errant keystrokes)

SEVERITY

Low

PROOF OF CONCEPT

Marketplace.sol

contract MarketPlace {

Does not inherit IMarketPlace, defined here

TOOLS USED

Manual Analysis

MITIGATION

-8: contract MarketPlace {
+8: contract MarketPlace is IMarketPlace {

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address.

Note: while it has been indicated by the sponsor input validation will be on the front-end side to relieve users from unnecessary gas spendings, this issue here concerns constructor functions, ie when the contract is deployed by the team.

SEVERITY

Low

PROOF OF CONCEPT

11 instances include:

Swivel.sol

marketPlace = m

MarketPlace.sol

creator = c

VaultTracker.sol

protocol = p
maturity = m
cTokenAddr = c
swivel = s

ZcToken.sol

protocol = _protocol
underlying = _underlying
maturity = _maturity
cToken = _cToken
redeemer = IRedeemer(_redeemer)

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the immutable variables aforementioned.

Safe.sol does not check contract existence

PROBLEM

Safe.sol is used to perform ERC20 transfers in the protocols contracts. It does not check for contract existence, meaning any call to an address with no bytecode (the address zero or an EOA) would not revert upon Safe.transfer. If there is a market with an underlying token being an empty bytecode address, this means a user can initiate a position without actually transferring any token to Swivel - as all protocols except Aave do not check for the underlying token. As only an admin can add a market, the risk of this issue affecting the protocols is low.

SEVERITY

Low

PROOF OF CONCEPT

In Safe.transfer() and Safe.transferFrom, success is set to 1 if the call returns 1, which is the case if it made to an address with no bytecode.

TOOLS USED

Manual Analysis

MITIGATION

Use extcodesize() in Safe.sol transfer functions to ensure the destination contract has bytecode.

Setters should check the input value

PROBLEM

Setters should check the input value - ie make revert if it is the zero address or zero

Note: while it has been indicated by the sponsor input validation will be on the front-end side to relieve users from unnecessary gas spendings, this issue here concerns constructor functions or setters called by authorized admins, not users, and most likely by interacting with the contract directly (for instance using Etherscan or Hardhat) and not with a front-end.

SEVERITY

Low

PROOF OF CONCEPT

5 instances:

Swivel.sol

function setAdmin(address a)

MarketPlace.sol

function setSwivel(address s)
function setAdmin(address a)

Creator.sol

function setAdmin(address a)
function setMarketPlace(address m)

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero checks to the setters aforementioned.

Wrong function name

PROBLEM

As per the docs, MarketPlace is to act as the IRedeemer to burn ZcTokens and call Swivel.authRedeem. The problem is that Swivel does not implement authRedeem, but authRedeemZcToken, meaning the functions call ISwivel(swivel).authRedeem(p, u, market.cTokenAddr, t, a) will always revert. A consequence of this is that ZcToken.withdraw and ZcToken.redeem will also always revert - that is, if ZcToken.redeemer is MarketPlace.

SEVERITY

Low

PROOF OF CONCEPT

Swivel.sol

function authRedeemZcToken(uint8 p, address u, address c, address t, uint256 a)

TOOLS USED

Manual Analysis

MITIGATION

Change the function name to authRedeem

2**256 - 1 can be re-written

PROBLEM

2**256 - 1 can be re-written as type(uint256).max

SEVERITY

Non-Critical

PROOF OF CONCEPT

VaultTracker.sol

uint256 max = 2**256 - 1

TOOLS USED

Manual Analysis

Constants instead of magic numbers

PROBLEM

It is best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

SEVERITY

Non-Critical

PROOF OF CONCEPT

30 instances:

VaultTracker.sol

yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26
uint256 interest = (yield * vlt.notional) / 1e26
yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26
uint256 interest = (yield * vlt.notional) / 1e26
yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26
uint256 interest = (yield * vlt.notional) / 1e26
yield = ((maturityRate * 1e26) / from.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / from.exchangeRate) - 1e26
uint256 interest = (yield * from.notional) / 1e26
yield = ((maturityRate * 1e26) / to.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / to.exchangeRate) - 1e26
uint256 newVaultInterest = (yield * to.notional) / 1e26
yield = ((maturityRate * 1e26) / sVault.exchangeRate) - 1e26
yield = ((exchangeRate * 1e26) / sVault.exchangeRate) - 1e26
uint256 newVaultInterest = (yield * sVault.notional) / 1e26

TOOLS USED

Manual Analysis

MITIGATION

Define constant variables for the literal values aforementioned.

Events indexing

PROBLEM

Events should use the maximum amount of indexed fields: up to three parameters. This makes it easier to filter for specific values in front-ends.

SEVERITY

Non-Critical

PROOF OF CONCEPT

3 instances:

lending-market/Comptroller.sol

event ScheduleWithdrawal(address indexed token, uint256 hold)
event ScheduleApproval(address indexed token, uint256 hold)
event ScheduleFeeChange(uint256 hold)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

5 instances:

Swivel.sol

function setAdmin(address a)

MarketPlace.sol

function setSwivel(address s)
function setAdmin(address a)

Creator.sol

function setAdmin(address a)
function setMarketPlace(address m)

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function order

PROBLEM

Functions should be ordered following the Solidity conventions

SEVERITY

Non-Critical

PROOF OF CONCEPT

Swivel.sol

• the modifier authorized is placed at the end of the contract, while it should be placed before every other function:

Inside each contract, library or interface, use the following order:

Type declarations

State variables

Events

Modifiers

Functions

MarketPlace.sol

• the modifiers authorized and unpaused are placed at the end of the contract, while it should be placed before every other function

• the internal function calculateReturn() is placed before several external functions, while it external functions should be declared before internal ones:

Functions should be grouped according to their visibility and ordered:

constructor

receive function (if exists)

fallback function (if exists)

external

public

internal

private

TOOLS USED

Manual Analysis

MITIGATION

Place the functions in the correct order.

Long lines

PROBLEM

Source codes lines should be limited to a certain number of characters. A good practice is to ensure the code does not require a horizontal scroll bar on GitHub. The lines mentioned below have that problem

SEVERITY

Non-Critical

PROOF OF CONCEPT

14 instances:

Swivel.sol

if (!mPlace.custodialInitiate(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, principalFilled)) { revert Exception(8, 0, 0, address(0), address(0)); }
if (!IMarketPlace(marketPlace).p2pZcTokenExchange(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, a)) { revert Exception(11, 0, 0, address(0), address(0)); }
if (!mPlace.p2pVaultExchange(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, principalFilled)) { revert Exception(12, 0, 0, address(0), address(0)); }
if (!IMarketPlace(marketPlace).p2pZcTokenExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, principalFilled)) { revert Exception(11, 0, 0, address(0), address(0)); }
if (!IMarketPlace(marketPlace).p2pVaultExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, a)) { revert Exception(12, 0, 0, address(0), address(0)); }
if (!mPlace.custodialExit(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, principalFilled)) { revert Exception(9, 0, 0, address(0), address(0)); }

MarketPlace.sol

function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount) {

ZcToken.sol

constructor(uint8 _protocol, address _underlying, uint256 _maturity, address _cToken, address _redeemer, string memory _name, string memory _symbol, uint8 _decimals)
return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
return (balanceOf[owner] * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);
return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));
/// @notice At or after maturity, burns exactly principalAmount of Principal Tokens from owner and sends underlyingAmount of underlying tokens to receiver.

TOOLS USED

Manual Analysis

MITIGATION

Split the lines to avoid needing a scroll bar to look through the code

Natspec

PROBLEM

Important functions should have a @notice comment to describe what they perform.

SEVERITY

Non-Critical

PROOF OF CONCEPT

MarketPlace.sol

function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken) returns (uint256 underlyingAmount)

TOOLS USED

Manual Analysis

MITIGATION

Add a @notice comment to this function

Non-library files should use fixed compiler versions

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

ZcToken.sol

pragma solidity ^0.8.4

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

Non-library files should use the same compiler version

PROBLEM

contracts within the scope should be compiled using the same compiler version.

SEVERITY

Non-Critical

PROOF OF CONCEPT

All the files in scope have the compiler version set to 0.8.13, while ZcToken.sol has it set to ^0.8.4 .

TOOLS USED

Manual Analysis

MITIGATION

Use the same compiler version throughout the contracts

open TODOs

PROBLEM

There are open TODOs in the code. Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

SEVERITY

Non-Critical

PROOF OF CONCEPT

16 instances:

lending-market/Comptroller.sol

// TODO immutable?
// TODO cheaper to assign amount here or keep the ADD?
// TODO assign amount or keep the ADD?
// TODO assign amount or keep ADD?
// TODO assign amount or keep the ADD?
// TODO assign amount or keep the ADD?
// TODO assign amount or keep the ADD?
// TODO assign amount or keep the ADD?
// TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
// TODO is Rari a drop in here?
// TODO explain the Aave deposit args
// TODO explain the 0 (primary account)
// TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
// TODO is Rari a drop in here?
// TODO explain the withdraw args
// TODO explain the 0

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

1 instance:

MarketPlace.sol

function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public

TOOLS USED

Manual Analysis

MITIGATION

Declare it as external instead of public

Tautological code

PROBLEM

Remove tautologies.

SEVERITY

Non-Critical

PROOF OF CONCEPT

5 instances:

Swivel.sol

return IYearn(c).deposit(a) >= 0
return IErc4626(c).deposit(a, address(this)) >= 0
return IYearn(c).withdraw(a) >= 0
return IAave(aaveAddr).withdraw(u, a, address(this)) >= 0
return IErc4626(c).withdraw(a, address(this), address(this)) >= 0

TOOLS USED

Manual Analysis

MITIGATION

Remove the tautologies. Simply replace them with a return true statement.

Typos

PROBLEM

There are a few typos in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

3 instances:

Swivel.sol

Varifies
it's signature.
withraw

TOOLS USED

Manual Analysis

MITIGATION

Correct the typos.

Gas Report

Table of Contents

• Address mappings can be combined in a single mapping
• Bytes constant are cheaper than string constants
• Caching storage variables in local variables to save gas
• Caching mapping accesses in local variables to save gas
• Calldata instead of memory for RO function parameters
• Clones for cheap contract deployment
• Constructor parameters should be avoided when possible
• Constants can be private
• Event fields are redundant
• Functions with access control cheaper if payable
• Prefix increments
• Storage cheaper than memory
• unchecked arithmetic
• unnecessary computation
• upgrade Solidity compiler version
• use Assembly for simple setters
• writing zero wastes gas

Address mappings can be combined in a single mapping

IMPACT

Combining mappings of address into a single mapping of address to a struct can save a Gssset (20000 gas) operation per mapping combined. This also makes it cheaper for functions reading and writing several of these mappings by saving a Gkeccak256 operation- 30 gas.

PROOF OF CONCEPT

1 instance:

Swivel.sol

mapping (address => uint256) public withdrawals
mapping (address => uint256) public approvals

TOOLS USED

Manual Analysis

MITIGATION

Combine the address mappings aforementionned in a single address => struct mapping, for instance

- mapping (address => uint256) public withdrawals;
  /// @dev maps a token address to a point in time, a hold, after which an approval can be made
- mapping (address => uint256) public approvals;
+ struct Hold {
+    uint256 withdrawals;
+    uint256 approvals;
+ }
+ mapping (address => Hold) public holds;

Bytes constant are cheaper than string constants

IMPACT

If the string can fit into 32 bytes, then bytes32 is cheaper than string. string is a dynamically sized-type, which has current limitations in Solidity compared to a statically sized variable. This means extra gas spent upon deployment and every time the constant is read.

PROOF OF CONCEPT

Instances:

Swivel.sol

string constant public NAME = 'Swivel Finance';
string constant public VERSION = '3.0.0';

TOOLS USED

Manual Analysis

MITIGATION

- string constant public NAME = 'Swivel Finance';
- string constant public VERSION = '3.0.0';
+ bytes32 constant public NAME = 'Swivel Finance';
+ bytes32 constant public VERSION = '3.0.0';

Caching storage variables in local variables to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length can yield significant gas savings if the array length is high

PROOF OF CONCEPT

2 instances:

Swivel.sol

scope: setFee()

• feeChange is read twice. Unless feeChange == 0, but given that it is an admin function, it is expected that when the admin invokes this function, all the required conditions are met, hence the state variable will be read twice from storage.

if (feeChange == 0)
if (block.timestamp < feeChange)

MarketPlace.sol

scope: createMarket()

• swivel is read twice. Unless swivel == address(0), but given that it is an admin function, it is expected that when the admin invokes this function, all the required conditions are met, hence the state variable will be read twice from storage.

if (swivel == address(0))
(address zct, address tracker) = ICreator(creator).create(p, underAddr, m, c, swivel, n, s, IErc20(underAddr).decimals())

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables using local variables.

Caching mapping accesses in local variables to save gas

IMPACT

Anytime you are reading from a mapping value more than once, it is cheaper in gas cost to cache it, by saving one gkeccak256 operation - 30 gas.

PROOF OF CONCEPT

8 instances:

Swivel.sol

scope: initiateVaultFillingZcTokenInitiate()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: initiateZcTokenFillingVaultInitiate()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: initiateZcTokenFillingZcTokenExit()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: initiateVaultFillingVaultExit()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: exitZcTokenFillingZcTokenInitiate()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: exitVaultFillingVaultInitiate()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: exitVaultFillingZcTokenExit()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

scope: exitZcTokenFillingVaultExit()

• filled[hash] is read twice:

uint256 amount = a + filled[hash];
filled[hash] += a;

TOOLS USED

Manual Analysis

MITIGATION

cache these filled[hash] accesses using local variables. Note: this will not save gas on the first call for a specific hash - only on subsequent calls.

With that in mind, considering all these functions will only be called once for a specific hash, you can do the following change to save SLOAD operations:

-uint256 amount = a + filled[hash];

-if (amount > o.premium) { revert Exception(5, amount, o.premium, address(0), address(0)); }
+if (a > o.premium) { revert Exception(5, a, o.premium, address(0), address(0)); }

-filled[hash] += a;
+filled[hash] = a;

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory,but it alleviates the compiler from the abi.decode() step that copies each index of the calldata to the memory index, each iteration costing 60 gas.

PROOF OF CONCEPT

2 instances:

Swivel.sol

function setFee(uint16[] memory i, uint16[] memory d)

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Clones for cheap contract deployment

IMPACT

There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .

With the standard way using the new keyword, each contract created contains the entire logic. Using proxies allow only the first implementation to contain the logic, saving deployment costs on subsequent instances deployed.

PROOF OF CONCEPT

Instances:

Creator.sol

address zct = address(new ZcToken(p, u, m, c, marketPlace, n, s, d))
address tracker = address(new VaultTracker(p, m, c, sw))

TOOLS USED

Manual Analysis

MITIGATION

Use a proxy system, see here for an example.

Constants can be private

IMPACT

Marking constants as private save gas upon deployment, as the compiler does not have to create getter functions for these variables. It is worth noting that a private variable can still be read using either the verified contract source code or the bytecode. For immutable variables written via constructor parameters, you can also look the contract deployment transaction.

PROOF OF CONCEPT

10 instances:

Swivel.sol

uint256 constant public HOLD = 3 days
uint16 constant public MIN_FEENOMINATOR = 33

Marketplace.sol

address public immutable creator

VaultTracker.sol

address public immutable admin
address public immutable cTokenAddr
address public immutable swivel
uint256 public immutable maturity
uint8 public immutable protocol

ZcToken.sol

uint8 public immutable protocol
address public immutable cToken

TOOLS USED

Manual Analysis

MITIGATION

Make these constants private instead of public

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

4 instances:

Swivel.sol

marketPlace = m
aaveAddr = a
feenominators = [200, 600, 400, 200]

MarketPlace.sol

creator = c

TOOLS USED

Manual Analysis

MITIGATION

Hardcode these variables with their initial value instead of writing them during contract deployment with constructor parameters.

Event fields are redundant

PROBLEM

block.timestamp and block.number are added to event information by default, explicitly adding them is a waste of gas.

PROOF OF CONCEPT

1 instance:

MarketPlace.sol

emit Mature(p, u, m, exchangeRate, block.timestamp)

TOOLS USED

Manual Analysis

MITIGATION

Remove the uint256 matured event field, as it always corresponds to block.timestamp.

Functions with access control cheaper if payable

PROBLEM

A function with access control marked as payable will lbe cheaper for legitimate callers: the compiler removes checks for msg.value, saving approximately 20 gas per function call.

PROOF OF CONCEPT

36 instances:

Swivel.sol

function setAdmin(address a) external authorized(admin)
function scheduleWithdrawal(address e) external authorized(admin)
function blockWithdrawal(address e) external authorized(admin)
function withdraw(address e) external authorized(admin)
function scheduleFeeChange() external authorized(admin)
function blockFeeChange() external authorized(admin)
function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin)
function scheduleApproval(address e) external authorized(admin)
function blockApproval(address e) external authorized(admin)
function approveUnderlying(address[] calldata u, address[] calldata c) external authorized(admin)
function authRedeemZcToken(uint8 p, address u, address c, address t, uint256 a) external authorized(marketPlace)

MarketPlace.sol

function setSwivel(address s) external authorized(admin)
function setAdmin(address a) external authorized(admin)
function createMarket(uint8 p,uint256 m,address c,string memory n,string memory s) external authorized(admin)
function mintZcTokenAddingNotional(uint8 p, address u, uint256 m, address t, uint256 a) external authorized(swivel)
function burnZcTokenRemovingNotional(uint8 p, address u, uint256 m, address t, uint256 a) external authorized(swivel)
function authRedeem(uint8 p, address u, uint256 m, address f, address t, uint256 a) public authorized(markets[p][u][m].zcToken)
function redeemZcToken(uint8 p, address u, uint256 m, address t, uint256 a) external authorized(swivel)
function redeemVaultInterest(uint8 p, address u, uint256 m, address t) external authorized(swivel)
function custodialInitiate(uint8 p, address u, uint256 m, address z, address n, uint256 a) external authorized(swivel)
function custodialExit(uint8 p, address u, uint256 m, address z, address n, uint256 a) external authorized(swivel)
function p2pZcTokenExchange(uint8 p, address u, uint256 m, address f, address t, uint256 a) external authorized(swivel)
function p2pVaultExchange(uint8 p, address u, uint256 m, address f, address t, uint256 a) external authorized(swivel)
function transferVaultNotionalFee(uint8 p, address u, uint256 m, address f, uint256 a) external authorized(swivel)
function pause(uint8 p, bool b) external authorized(admin)

Creator.sol

function create ( //args ) external authorized(marketPlace)
function setAdmin(address a) external authorized(admin)
function setMarketPlace(address m) external authorized(admin)

VaultTracker.sol

function addNotional(address o, uint256 a) external authorized(admin)
function removeNotional(address o, uint256 a) external authorized(admin)
function redeemInterest(address o) external authorized(admin)
function matureVault(uint256 c) external authorized(admin)
function transferNotionalFrom(address f, address t, uint256 a) external authorized(admin)
function transferNotionalFee(address f, uint256 a) external authorized(admin)

ZcToken.sol

function burn(address f, uint256 a) external onlyAdmin(address(redeemer))
function mint(address t, uint256 a) external onlyAdmin(address(redeemer))

TOOLS USED

Manual Analysis

MITIGATION

Remove these event fields.

Immutable variables save storage

PROBLEM

If a variable is set in the constructor and never modified afterwrds, marking it as immutable can save a storage slot - 20,000 gas. This also saves 97 gas on every read access of the variable.

PROOF OF CONCEPT

1 instance:

Swivel.sol

address public aaveAddr
aaveAddr is set in the constructor and read in two functions, but never modified.

TOOLS USED

Manual Analysis

MITIGATION

Mark aaveAddr as immutable.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops.

PROOF OF CONCEPT

5 instances:

xTRIBE.sol

unchecked {i++;}
unchecked {i++;}
unchecked {i++;}
unchecked {x++;}
unchecked {i++;}

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Storage cheaper than memory

PROBLEM

Reference types cached in memory cost more gas than using storage, as new memory is allocated for these variables, copying data from storage to memory for each field of the struct or array: this means every field of the struct/array is read.

PROOF OF CONCEPT

18 instances:

MarketPlace.sol

Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]
Market memory market = markets[p][u][m]

VaultTracker.sol

Vault memory vlt = vaults[o]
Vault memory vlt = vaults[o]
Vault memory vlt = vaults[o]
Vault memory from = vaults[f]
Vault memory to = vaults[t]
Vault memory oVault = vaults[f]
Vault memory sVault = vaults[swivel]
Vault memory vault = vaults[o]

TOOLS USED

Manual Analysis

MITIGATION

Use storage instead of memory. Cache any field read more than once onto the stack to avoid unnecessary SLOAD operations.

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances:

VaultTracker.sol

vlt.notional -= a
Because of the check here, it cannot underflow

from.notional -= a
Because of the check here, it cannot underflow

ZcToken.sol

allowance[holder][msg.sender] -= previewAmount
Because of the check here, it cannot underflow

allowance[holder][msg.sender] -= principalAmount
Because of the check here, it cannot underflow

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block

Unnecessary computation

IMPACT

Unnecessary stack variables can be removed to save gas

PROOF OF CONCEPT

Instances:

ZcToken.sol

111:             uint256 allowed = allowance[holder][msg.sender];
112:             if (allowed >= previewAmount) {
113:                 revert Approvals(allowed, previewAmount);
114:             }
115:             allowance[holder][msg.sender] -= previewAmount;

132:             uint256 allowed = allowance[holder][msg.sender];
133:             if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
134:             allowance[holder][msg.sender] -= principalAmount

Both allowed are not necessary. They only save gas if the function reverts

TOOLS USED

Manual Analysis

MITIGATION

Remove both allowed and read the storage variables directly in the condition blocks.

Upgrade Solidity compiler version

IMPACT

• 0.8.10 removes contract existence checks if the external call has a return value - 700 gas

PROOF OF CONCEPT

Instances:

ERC20Gauges.sol

pragma solidity ^0.8.4

MITIGATION

Upgrade ZcToken.sol compiler version.

use Assembly for simple setters

IMPACT

Where it does not affect readability, using assembly allows to save gas not only on deployment, but also on function calls. This is the case for instance for simple admin setters.

PROOF OF CONCEPT

Instances:

Swivel.sol

428:   function setAdmin(address a) external authorized(admin) returns (bool) {
429:     admin = a;
430: 
431:     return true;
432:   }

MarketPlace.sol

53:   function setAdmin(address a) external authorized(admin) returns (bool) {
54:     admin = a;
55:     return true;
56:   }

Creator.sol

47:   function setAdmin(address a) external authorized(admin) returns (bool) {
48:     admin = a;
49:     return true;
50:   }

MITIGATION

-  admin = a;
+  assembly {
+    sstore(admin.slot, a)
+  }

writing zero wastes gas

IMPACT

Swivel.sol uses a timelock system for changing the fees, which consists in setting feeChange to zero every time fees are changed, then setting it to block.timestamp + HOLD upon the next scheduled fee change. As SSTORE operations are more expensive when setting a state variable from zero to a non-zero value than setting it from a non-zero value to another non-zero value, gas can be saved by not setting feeChange to zero upon a fee update.

PROOF OF CONCEPT

Instances:

Swivel.sol

• in blockFeeChange: feeChange = 0

• in setFee: feeChange = 0

MITIGATION

Change to feeChange = 1, and change the following line in setFee:

- if (feeChange == 0)
+ if (feeChange == 1)