search

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

0 stars 1 forks source link

QA Report #200

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

Low

Non-critical

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

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)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L137)\
[    if (!IMarketPlace(marketPlace).p2pZcTokenExchange(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, a)) { revert Exception(11, 0, 0, address(0), address(0)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L205)\
[    if (!mPlace.p2pVaultExchange(o.protocol, o.underlying, o.maturity, o.maker, msg.sender, principalFilled)) { revert Exception(12, 0, 0, address(0), address(0)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L229)\
[    if (!IMarketPlace(marketPlace).p2pZcTokenExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, principalFilled)) { revert Exception(11, 0, 0, address(0), address(0)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L301)\
[    if (!IMarketPlace(marketPlace).p2pVaultExchange(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, a)) { revert Exception(12, 0, 0, address(0), address(0)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L331)\
[    if (!mPlace.custodialExit(o.protocol, o.underlying, o.maturity, msg.sender, o.maker, principalFilled)) { revert Exception(9, 0, 0, address(0), address(0)); }](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L399)

### 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) {](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L148)

### ZcToken.sol

[    constructor(uint8 _protocol, address _underlying, uint256 _maturity, address _cToken, address _redeemer, string memory _name, string memory _symbol, uint8 _decimals) ](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L31)\
[        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L47)\
[        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L56)\
[        return (principalAmount * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L74)\
[        return (balanceOf[owner] * IRedeemer(redeemer).getExchangeRate(protocol, cToken) / IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate);](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L83)\
[        return (underlyingAmount * IRedeemer(redeemer).markets(protocol, underlying, maturity).maturityRate / IRedeemer(redeemer).getExchangeRate(protocol, cToken));](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L92)\
[    /// @notice At or after maturity, burns exactly `principalAmount` of Principal Tokens from `owner` and sends underlyingAmount of underlying tokens to `receiver`.](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L120)

## 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)](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L148)

## 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](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/ZcToken.sol#L2)

## 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?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L33)\
[// TODO cheaper to assign amount here or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L120)\
[// TODO assign amount or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L192)\
[// TODO assign amount or keep ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L221)\
[// TODO assign amount or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L286)\
[// TODO assign amount or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L317)\
[// TODO assign amount or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L347)\
[// TODO assign amount or keep the ADD?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#382)\
[// TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L707)\
[// TODO is Rari a drop in here?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L708)\
[// TODO explain the Aave deposit args](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L716)\
[// TODO explain the 0 (primary account)](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L721)\
[// TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L740)\
[// TODO is Rari a drop in here?](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L741)\
[// TODO explain the withdraw args](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L748)\
[// TODO explain the 0](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L752)

## 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](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L148)

## 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](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L712)\
[return IErc4626(c).deposit(a, address(this)) >= 0](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L727)\
[return IYearn(c).withdraw(a) >= 0](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L745)\
[return IAave(aaveAddr).withdraw(u, a, address(this)) >= 0](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L749)\
[return IErc4626(c).withdraw(a, address(this), address(this)) >= 0](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L757)

## 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](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L682)\
[it's signature.](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L682)\
[withraw](https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L747)

## TOOLS USED

Manual Analysis

## MITIGATION

Correct the typos.
robrobbins commented 2 years ago

contracts should explicitly implement their interface (if available). agreed. addressed here: https://github.com/Swivel-Finance/gost/pull/424

robrobbins commented 2 years ago

reference to VaultTracker having 2**256-1 is incorrect. Is in Swivel.sol. agree with assessment tho, changed.

robrobbins commented 2 years ago

casting int returns as booleans is not tautological, particularly in the context of what that method is doing (normalizing returns)

robrobbins commented 2 years ago

MarketPlace.authRedeem as external agree - changed

robrobbins commented 2 years ago

removing TODOS agree: removing

robrobbins commented 2 years ago

various issues above addressed here: https://github.com/Swivel-Finance/gost/pull/425

joestakey commented 2 years ago

The Wrong Function Name issue is the same as the one in #39, describing why ZcToken.withdraw always reverts. Shouldn't it be upgraded as a duplicate of M-03 ? @bghughes

robrobbins commented 2 years ago

adding maybe in order to discuss adding events for setters

joestakey commented 2 years ago

The Wrong Function Name issue is the same as the one in #39, describing why ZcToken.withdraw always reverts. Shouldn't it be upgraded as a duplicate of #39 ? @bghughes

@0xean what do you think?

0xean commented 2 years ago

I am not opposed to upgrading and creating a duplicate, but the warden doesn't identify the impact of this correctly which I think is important for an issue to be marked as high severity.

robrobbins commented 2 years ago

event is now in place for setAdmin the only one that is needed. the other setters are one-time only by the deploying admin.