QA Report

[code423n4]

• Low
  • 1. Lack of ACK during owner change
  • 2. Lack of EOAs check
  • 3. Outdated compiler
  • 4. Unify errors
  • 5. Emit event for important state changes
  • 6. Ensure that ecrecover returns is not address0
• Non critical
  • 7. Open TODO

Low

1. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• Creator.sol#L47

2. Lack of EOAs check

The following lines require a contract as an argument, in some of them, once the value is set, it cannot be changed again, so it is mandatory to check if these values are as expected.

Affected source code:

• Creator.sol#L59
• MarketPlace.sol#L48

3. Outdated compiler

The pragma version used is:

pragma solidity ^0.8.4;
pragma solidity 0.8.13;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

4. Unify errors

The transferNotionalFee method of VaultTracker does not check that the argument overflows, as other methods in the same contract do. The logic must be unified.

+   if (a > oVault.notional) { revert Exception(31, a, oVault.notional, address(0), address(0)); }
    // remove notional from its owner
    oVault.notional -= a;

5. Emit event for important state changes

It's important to emit events when the governance change important values in the contract, in order to be more open and transparent.

For example, when a contract is paused or unpaused like in the following lines:

• MarketPlace.sol#L338

6. Ensure that ecrecover returns is not address(0)

The method ecrecover returns address(0) when the signature is wrong, so if a user use address(0) as a maker the return will be true.

Affected source code:

• Swivel.sol#L695

Non critical

7. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

• Swivel.sol#L120
• Swivel.sol#L157
• Swivel.sol#L192
• Swivel.sol#L221
• Swivel.sol#L286
• Swivel.sol#L317
• Swivel.sol#L347
• Swivel.sol#L382
• Swivel.sol#L707-L721
• Swivel.sol#L740-L752

[robrobbins]

agree with 4. addressed: https://github.com/Swivel-Finance/gost/pull/432

[robrobbins]

address(0) in the sig lib was addressed elsewhere