Skip to content

Latest commit

 

History

History
497 lines (341 loc) · 32.1 KB

39.md

File metadata and controls

497 lines (341 loc) · 32.1 KB
Raw
title sponsor slug date findings contest
Swivel contest
Swivel
2021-09-swivel
2021-11-05
39

Overview

About C4

Code 432n4 (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 code contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the code contest outlined in this document, C4 conducted an analysis of Swivel contest smart contract system written in Solidity. The code contest took place between September 30—October 6 2021.

Wardens

15 Wardens contributed reports to the Swivel contest code contest:

This contest was judged by 0xean.

Final report assembled by CloudEllie and itsmetechjay.

Summary

The C4 analysis yielded an aggregated total of 27 unique vulnerabilities and 76 total findings. All of the issues presented here are linked back to their original finding.

Of these vulnerabilities, 4 received a risk rating in the category of HIGH severity, 5 received a risk rating in the category of MEDIUM severity, and 18 received a risk rating in the category of LOW severity.

C4 analysis also identified 20 non-critical recommendations and 28 gas optimizations.

Scope

The code under review can be found within the C4 Swivel contest code repository, is composed of 3 smart contracts written in the Solidity programming language, and includes 996 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.

Vulnerabilities are divided into three primary risk categories: high, medium, and low.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.

High Risk Findings (4)

[H-01] Unsafe handling of underlying tokens

Submitted by 0xsanson, also found by 0xRajeev, cmichel, defsec, GalloDaSballo, JMukesh, leastwood, loop, nikitastupin, pants, and pauliax.

Impact

Not every ERC20 token follows OpenZeppelin's recommendation. It's possible (inside ERC20 standard) that a transferFrom doesn't revert upon failure but returns false.

The code doesn't check these return values. For example uToken.transferFrom(msg.sender, o.maker, a); in initiateVaultFillingZcTokenInitiate can be exploited by the msg.sender to initiate a trade without sending any underlying.

Proof of Concept

grep 'transfer' Swivel.sol

Tools Used

editor

Recommended Mitigation Steps

Consider using OpenZeppelin's library with safe versions of transfer functions.

[H-02] Swivel: Taker is charged fees twice in exitVaultFillingVaultInitiate

Submitted by itsmeSTYJ, also found by gpersoon.

Impact

Taker is charged fees twice in exitVaultFillingVaultInitiate() . Maker is transferring less than premiumFilled to taker and then taker is expected to pay fees i.e. taker's net balance is premiumFilled - 2*fee

Recommended Mitigation Steps

function exitVaultFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
  bytes32 hash = validOrderHash(o, c);

  require(a <= (o.principal - filled[hash]), 'taker amount > available volume');
  
  filled[hash] += a;
      
  uint256 premiumFilled = (((a * 1e18) / o.principal) * o.premium) / 1e18;
  uint256 fee = ((premiumFilled * 1e18) / fenominator[3]) / 1e18;

  Erc20 uToken = Erc20(o.underlying);
  // transfer premium from maker to sender
  uToken.transferFrom(o.maker, msg.sender, premiumFilled);

  // transfer fee in underlying to swivel from sender
  uToken.transferFrom(msg.sender, address(this), fee);

  // transfer <a> vault.notional (nTokens) from sender to maker
  require(MarketPlace(marketPlace).p2pVaultExchange(o.underlying, o.maturity, msg.sender, o.maker, a), 'vault exchange failed');

  emit Exit(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled);
}

JTraversa (Swivel) confirmed

0xean (judge) commented:

Based on

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

This is being upgraded to a high risk. The duplicate of it was at that level by the submitting warden and considering that fees are being incorrectly taken from the taker and not the maker, the maker ends up with a higher balance than expected and the taker has no way to recoup these fees (assets are now lost).

JTraversa (Swivel) commented:

Is that how it is interpreted? I'd assume that high risk would imply a valid attack path that a user could use to drain deposited funds based on that description.

I won't fight this one obviously, just think there's a clear differentiation between this and the other high risk issue.

[H-03] transferNotionalFrom doesn't check from != to

Submitted by gpersoon, also found by cmichel.

Impact

The function transferNotionalFrom of VaultTracker.sol uses temporary variables to store the balances. If the "from" and "to" address are the same then the balance of "from" is overwritten by the balance of "to". This means the balance of "from" and "to" are increased and no balances are decreased, effectively printing money.

Note: transferNotionalFrom can be called via transferVaultNotional by everyone.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/vaulttracker/VaultTracker.sol#L144-L196

  function transferNotionalFrom(address f, address t, uint256 a) external onlyAdmin(admin) returns (bool) {
  Vault memory from = vaults\[f];
  Vault memory to = vaults\[t];
  ...
  vaults\[f] = from;
  ...
  vaults\[t] = to;    // if f==t then this will overwrite vaults\[f]

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L234-L238

  function transferVaultNotional(address u, uint256 m, address t, uint256 a) public returns (bool) {
  require(VaultTracker(markets\[u]\[m].vaultAddr).transferNotionalFrom(msg.sender, t, a), 'vault transfer failed');

Tools Used

Recommended Mitigation Steps

Add something like the following: require (f != t,"Same");

JTraversa (Swivel) confirmed

[H-04] return value of 0 from ecrecover not checked

Submitted by gpersoon, also found by 0xRajeev, cmichel, and nikitastupin.

Impact

The solidity function ecrecover is used, however the error result of 0 is not checked for. See documentation: https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions "recover the address associated with the public key from elliptic curve signature or return zero on error. "

Now you can supply invalid input parameters to the Sig.recover function, which will then result 0. If you also set o.maker to be 0 then this will match and an invalid signature is not detected.

So you can do all kinds of illegal & unexpected transactions.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L476-L484

  function validOrderHash(Hash.Order calldata o, Sig.Components calldata c) internal view returns (bytes32) {
  ...
  require(o.maker == Sig.recover(Hash.message(domain, hash), c), 'invalid signature');
  return hash;
  }

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Sig.sol#L16-L23

  function recover(bytes32 h, Components calldata c) internal pure returns (address) {
  ...
  return ecrecover(h, c.v, c.r, c.s);

Tools Used

Recommended Mitigation Steps

Verify that the result from ecrecover isn't 0

JTraversa (Swivel) acknowledged JTraversa (Swivel) commented:

Id say this is noteable, but because all actions require approvals from o.maker, having 0x00 as o.maker with an "invalid" but valid signature should not be impactful. The suggestion would be to filter 0x00 makers from the orderbook? (which we do)

Medium Risk Findings (5)

[M-01] Admin is a single-point of failure without any mitigations

Submitted by 0xRajeev, also found by 0xsanson and leastwood.

Impact

Admin role has absolute power across Swivel, Marketplace and VaultTracker contracts with several onlyOwner functions. There is no ability to change admin to a new address or renounce it which is helpful for lost/compromised admin keys or to delegate control to a different governance/DAO address in future.

The project does not use the widely used OpenZeppelin Ownable library which provides transfer/renounce functions to mitigate such compromised/accidental situations with admin keys. This makes admin role/key a single-point of failure.

Proof of Concept

Tools Used

Manual Analysis

Recommended Mitigation Steps

Ensure admins are reasonably redundant/independent (3/7 or 5/9) multisigs and add transfer/renounce functionality for admin. Consider using OpenZeppelin’s Ownable library.

JTraversa (Swivel) acknowledged:

Similar to some other suggestions, i'm personally not sure if this is within the scope of the competition / an "issue". We have immediate plans to add admin transfer functionality however left it out of the scope of this audit as not having a transferAdmin function doesn't seem to be any more dangerous than the admin having the functionality it currently has.

That said, in production the admin will both be a multisig and there is also basic admin timelock/transfer functionality, with on-chain governance as opposed to centralized-ish multisigs coming Q1.

[M-02] Missing event & timelock for critical onlyAdmin functions

Submitted by 0xRajeev, also found by defsec.

Impact

onlyAdmin functions that change critical contract parameters/addresses/state should emit events and consider adding timelocks so that users and other privileged roles can detect upcoming changes (by offchain monitoring of events) and have the time to react to them.

Privileged functions in all contracts, for e.g. VaultTracker onlyAdmin, have direct financial or trust impact on users who should be given an opportunity to react to them by exiting/engaging without being surprised when changes initiated by such functions are made effective opaquely (without events) and/or immediately (without timelocks).

See similar Medium-severity finding in ConsenSys's Audit of 1inch Liquidity Protocol (https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing)

Proof of Concept

and others

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add events to all possible flows (some flows emit events in callers) and consider adding timelocks to such onlyAdmin functions.

0xean (judge) commented:

removing duplicate mark since the (valid) suggestion to emit events on the changes made by admin calls

[M-03] Previously created markets can be overwritten

Submitted by 0xRajeev, also found by 0xsanson, cmichel, gpersoon, itsmeSTYJ, and pauliax.

Impact

The createMarket function allows accidental overwriting of previously created markets for the same combination of underlying and maturity timestamp (u, m) because there is no zero-address check to see if a previously created market exists for that combination.

Proof of Concept

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add a zero-address check for markets\[u]\[m] in createMarket before writing to it

[M-04] fee-on-transfer underlying can cause problems

Submitted by 0xsanson.

Impact

The current implementation doesn't work with fee-on-transfer underlying tokens. Considering that Compound can have these kind of tokens (ex. USDT can activate fees), this issue can affect the protocol.

The problem arises when transferring tokens, basically blocking all functions in Swivel.sol for that particular token, since the contract wrongly assumes balances values. This becomes particularly problematic in the following scenario: a market for USDT is running without problems, then they activate the fee: this effectively blocks users from redeeming the underlying.

Proof of Concept

grep 'transfer' Swivel.sol for a complete list of affected lines (basically every tranfer or transferFrom of underlying tokens). Also grep 'redeemUnderlying' Swivel.sol.

For example:

  require(CErc20(mPlace.cTokenAddress(u, m)).redeemUnderlying(redeemed) == 0, 'compound redemption failed');
  // transfer underlying back to msg.sender
  Erc20(u).transfer(msg.sender, redeemed);

This would fail (revert) since the contract would have received less than redeemed tokens.

Tools Used

editor

Recommended Mitigation Steps

If the protocol wants to use all possible Compound tokens, a way to handle these tokens must be implemented. A possible way to do it is to check the balance of the contract before and after every time a token is transferred to see the effective quantity. To help keeping the code clear, a function like Compound's doTransferIn can be implemented.

JTraversa (Swivel) acknowledged:

Will review further. I dont believe that any tokens on compound currently have fees. Although it is news to me that USDT has toggle-able fees, whoops.

That said, given we have admin control over added assets, I'd probably also lower this to a low-risk if accepted.

0xean (judge) commented:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

based on this "leaking value" I would say it qualifies as a med-severity.

JTraversa (Swivel) commented:

We can account for the transfers in with a similar balance before transferFrom, and balance after check, in order to prevent additional deposits after a fee has been turned on.

That said, Im not sure we can account for redeemUnderlying easily because should a fee be turned on, all funds would just be stuck in our contract if we added a similar check? (a != balance2-balance1)

If a fee is turned on for USDT markets, there would be lost fee value, so if adding a check wouldn't work, the most reasonable response is just to make sure the market is pausable.

[M-05] Swivel: implementation for initiateZcTokenFillingZcTokenExit is incorrect

Submitted by itsmeSTYJ.

Impact

In initiateZcTokenFillingZcTokenExit() , this comment // transfer underlying tokens - the premium paid + fee in underlying to swivel (from sender) is incorrect because you are actually transferring the underlying tokens - premium paid to the maker (from sender) AND you have to pay fee separately to swivel.

initiateZCTokenFillingZcTokenExit means I want to sell my nTokens so that means a is the amount of principal I want to fill. Let's use a hypothetical example where I (taker) wants to fill 10 units of ZcTokenExit for maker.

  1. I transfer 10 units of underlying to Swivel. The net balances are: me (-a), swivel (+a)
  2. I transfer fee (in underlying) to Swivel. The net balances are: me (-a-fee), swivel (+a+fee)
  3. Swivel initiates my position, sends me the ZcToken and sends Maker the nTokens
  4. Maker pays me premiumFilled for the nTokens. The net balances are: me (-a-fee+premiumsFilled), swivel (+a+fee), maker (-premiumsFilled)
  5. Maker closes position. The net balances are: me (-a-fee+premiumsFilled), swivel (+fee), maker (-premiumsFilled+a)

So effectively, I (taker) should be paying a-premium to maker and fee to swivel.

Recommended Mitigation Steps

function initiateZcTokenFillingZcTokenExit(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal {
  bytes32 hash = validOrderHash(o, c);

  require(a <= o.principal - filled[hash]), 'taker amount > available volume'); // Note: you don't need to wrap these in brackets because if you look at the https://docs.soliditylang.org/en/latest/cheatsheet.html#order-of-precedence-of-operators, subtraction will always go before comparison 

  filled[hash] += a;

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

  // transfer underlying tokens - the premium paid in underlying to maker (from sender)
  Erc20(o.underlying).transferFrom(msg.sender, o.maker, a - premiumFilled);
  Erc20(o.underlying).transferFrom(msg.sender, swivel, fee);
  // transfer <a> zcTokens between users in marketplace
  require(MarketPlace(marketPlace).p2pZcTokenExchange(o.underlying, o.maturity, o.maker, msg.sender, a), 'zcToken exchange failed');
          
  emit Initiate(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled);
}

JTraversa (Swivel) confirmed:

Really good eye.

We had o.maker send the fee after receiving it (same # of transfers as suggested mitigation) and actually accidentally lost that in a larger organizational commit 😅 .

Low Risk Findings (18)

Non-Critical Findings (20)

Gas Optimizations (28)

Disclosures

C4 is an open organization governed by participants in the community.

C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.