search

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

0 stars 1 forks source link

QA Report #102

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

ISSUE LIST

C4-001 : Critical changes should use two-step procedure - Non Critical

C4-002 : Low level calls with solidity version 0.8.14 can result in optimiser bug. - LOW

C4-003 : The Contract Should safeApprove(0) first - LOW

C4-004 : Use of Block.timestamp - Non-critical

C4-005 : Incompatibility With Rebasing/Deflationary/Inflationary tokens - LOW

C4-006: Centralization Risk - LOW

C4-007: Open TODOs - Non-critical

C4-008 : Implement check effect interaction - Non Critical

ISSUES

C4-001 : Critical changes should use two-step procedure

Impact - NON CRITICAL

The critical procedures should be two step process. The contracts inherit OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role. The current ownership transfer process involves the current owner calling Unlock.transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier. Lack of two-step procedure for critical operations leaves them error-prone if the address is incorrect, the new address will take on the functionality of the new role immediately

for Ex : -Alice deploys a new version of the whitehack group address. When she invokes the whitehack group address setter to replace the address, she accidentally enters the wrong address. The new address now has access to the role immediately and is too late to revert

Proof of Concept

  1. Navigate to the following contract.
https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L53
https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L45

Tools Used

Code Review

Recommended Mitigation Steps

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

C4-002 : Low level calls with solidity version 0.8.x can result in optimiser bug.

Impact

The protocol is using low level calls with solidity version 0.8.x which can result in optimizer bug.

https://medium.com/certora/overly-optimistic-optimizer-certora-bug-disclosure-2101e3f7994d

Proof of Concept

  2022-07-swivel-main/Creator/Erc20.sol::4 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Creator/FixedPointMathLib.sol::2 => pragma solidity >=0.8.0;
  2022-07-swivel-main/Creator/IERC5095.sol::2 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Creator/IRedeemer.sol::2 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Creator/LibCompound.sol::2 => pragma solidity >=0.8.4;
  2022-07-swivel-main/Creator/ZcToken.sol::2 => pragma solidity ^0.8.4;
  2022-07-swivel-main/Marketplace/Erc20.sol::4 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Marketplace/FixedPointMathLib.sol::2 => pragma solidity >=0.8.0;
  2022-07-swivel-main/Marketplace/LibCompound.sol::2 => pragma solidity >=0.8.4;
  2022-07-swivel-main/Tokens/Erc20.sol::4 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Tokens/FixedPointMathLib.sol::2 => pragma solidity >=0.8.0;
  2022-07-swivel-main/Tokens/IERC5095.sol::2 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Tokens/IRedeemer.sol::2 => pragma solidity ^0.8.0;
  2022-07-swivel-main/Tokens/LibCompound.sol::2 => pragma solidity >=0.8.4;
  2022-07-swivel-main/Tokens/ZcToken.sol::2 => pragma solidity ^0.8.4;
  2022-07-swivel-main/VaultTracker/FixedPointMathLib.sol::2 => pragma solidity >=0.8.0;
  2022-07-swivel-main/VaultTracker/LibCompound.sol::2 => pragma solidity >=0.8.4;

Tools Used

Code Review

Recommended Mitigation Steps

Consider upgrading to solidity 0.8.15.

C4-003 : The Contract Should safeApprove(0) first - LOW

Impact

Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

  2022-07-swivel-main/Swivel/Swivel.sol::562 => Safe.approve(uToken, c[i], max);

When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.

Proof of Concept

(https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/LiquidityReserve.sol#L81)

Tools Used

None

Recommended Mitigation Steps

Approve with a zero amount first before setting the actual amount.

C4-004 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

  1. Navigate to the following contract.
2022-07-swivel-main/Creator/ZcToken.sol:44:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:53:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:62:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:71:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:80:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:89:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:101:        if (block.timestamp < maturity) {
2022-07-swivel-main/Creator/ZcToken.sol:126:        if (block.timestamp < maturity) { revert Maturity(maturity); }
2022-07-swivel-main/Creator/Erc20.sol:135:        if (deadline < block.timestamp) {
2022-07-swivel-main/Creator/Erc20.sol:136:            revert Deadline(deadline, block.timestamp);
2022-07-swivel-main/Swivel/Swivel.sol:437:    uint256 when = block.timestamp + HOLD;
2022-07-swivel-main/Swivel/Swivel.sol:462:    if (block.timestamp < when) { revert Exception(17, block.timestamp, when, address(0), address(0)); }
2022-07-swivel-main/Swivel/Swivel.sol:474:    uint256 when = block.timestamp + HOLD;
2022-07-swivel-main/Swivel/Swivel.sol:502:    if (block.timestamp < feeChange) { revert Exception(17, block.timestamp, feeChange, address(0), address(0)); }
2022-07-swivel-main/Swivel/Swivel.sol:523:    uint256 when = block.timestamp + HOLD;
2022-07-swivel-main/Swivel/Swivel.sol:557:      if (block.timestamp < when) { revert Exception(17, block.timestamp, when, address(0), address(0));
2022-07-swivel-main/Swivel/Swivel.sol:691:    if (o.expiry < block.timestamp) { revert Exception(3, o.expiry, block.timestamp, address(0), address(0)); }
2022-07-swivel-main/Marketplace/Erc20.sol:135:        if (deadline < block.timestamp) {
2022-07-swivel-main/Marketplace/Erc20.sol:136:            revert Deadline(deadline, block.timestamp);
2022-07-swivel-main/Marketplace/MarketPlace.sol:95:    if (block.timestamp < m) { revert Exception(24, block.timestamp, m, address(0), address(0)); }
2022-07-swivel-main/Marketplace/MarketPlace.sol:104:    emit Mature(p, u, m, exchangeRate, block.timestamp);
2022-07-swivel-main/Tokens/ZcToken.sol:44:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:53:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:62:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:71:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:80:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:89:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:101:        if (block.timestamp < maturity) {
2022-07-swivel-main/Tokens/ZcToken.sol:126:        if (block.timestamp < maturity) { revert Maturity(maturity); }
2022-07-swivel-main/Tokens/Erc20.sol:135:        if (deadline < block.timestamp) {
2022-07-swivel-main/Tokens/Erc20.sol:136:            revert Deadline(deadline, block.timestamp);

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-005 : Incompatibility With Rebasing/Deflationary/Inflationary tokens

Impact - LOW

PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

  1. Navigate to the following contract.
  2022-07-swivel-main/Swivel/Swivel.sol::125 => Safe.transferFrom(uToken, msg.sender, o.maker, a);
  2022-07-swivel-main/Swivel/Swivel.sol::128 => Safe.transferFrom(uToken, o.maker, address(this), principalFilled);
  2022-07-swivel-main/Swivel/Swivel.sol::163 => Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
  2022-07-swivel-main/Swivel/Swivel.sol::167 => Safe.transferFrom(uToken, msg.sender, address(this), (a + fee));
  2022-07-swivel-main/Swivel/Swivel.sol::199 => Safe.transferFrom(uToken, msg.sender, o.maker, a - premiumFilled);
  2022-07-swivel-main/Swivel/Swivel.sol::202 => Safe.transferFrom(uToken, msg.sender, address(this), fee);
  2022-07-swivel-main/Swivel/Swivel.sol::224 => Safe.transferFrom(IErc20(o.underlying), msg.sender, o.maker, a);
  2022-07-swivel-main/Swivel/Swivel.sol::293 => Safe.transferFrom(uToken, o.maker, msg.sender, principalFilled - a);
  2022-07-swivel-main/Swivel/Swivel.sol::298 => Safe.transferFrom(uToken, msg.sender, address(this), fee);
  2022-07-swivel-main/Swivel/Swivel.sol::324 => Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
  2022-07-swivel-main/Swivel/Swivel.sol::328 => Safe.transferFrom(uToken, msg.sender, address(this), fee);
  2022-07-swivel-main/Swivel/Swivel.sol::359 => Safe.transfer(uToken, o.maker, a - premiumFilled);
  2022-07-swivel-main/Swivel/Swivel.sol::363 => Safe.transfer(uToken, msg.sender, premiumFilled - fee);
  2022-07-swivel-main/Swivel/Swivel.sol::395 => Safe.transfer(uToken, msg.sender, principalFilled - a - fee);
  2022-07-swivel-main/Swivel/Swivel.sol::396 => Safe.transfer(uToken, o.maker, a);
  2022-07-swivel-main/Swivel/Swivel.sol::467 => Safe.transfer(token, admin, token.balanceOf(address(this)));
  2022-07-swivel-main/Swivel/Swivel.sol::562 => Safe.approve(uToken, c[i], max);
  2022-07-swivel-main/Swivel/Swivel.sol::580 => Safe.transferFrom(uToken, msg.sender, address(this), a);
  2022-07-swivel-main/Swivel/Swivel.sol::609 => Safe.transfer(IErc20(u), msg.sender, a);
  2022-07-swivel-main/Swivel/Swivel.sol::624 => Safe.transfer(IErc20(u), t, a);
  2022-07-swivel-main/Swivel/Swivel.sol::642 => Safe.transfer(IErc20(u), msg.sender, redeemed);
  2022-07-swivel-main/Swivel/Swivel.sol::661 => Safe.transfer(IErc20(u), msg.sender, redeemed);

Tools Used

Manual Code Review

Recommended Mitigation Steps

C4-006 : Centralization Risk

Impact - LOW

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.

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.

C4-007 : Open TODOs

Impact - LOW

Open TODOs can point to architecture or programming issues that still need to be resolved.

Location

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L120

Recommended Mitigation Steps

Consider resolving the TODOs before deploying.

C4-008 : Implement check effect interaction

Impact - LOW

There is no impact to the funds but to align with best practices, it is always better to update internal state before any external function calls.

Location

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L366

Recommended Mitigation Steps

Call external interaction before accounting. (Update the internal accounting before transferring the tokens out.)

robrobbins commented 1 year ago

we don't use any --ir type settings in the --optimize steps.

others are already labeled wontfix or addressed elsewhere