Swivel v3 contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L132-L134 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L111-L115

Vulnerability details

Impact

The logic around the decrementing the allowance in the withdraw and redeem methods of the contract ZcToken are wrong implemented and cannot be used.

Proof of Concept

There are a Denial of Service in the withdraw and redeem methods of the ZcToken contract, the allowance is checked in the opposite way, so if the allowed amount is greater or equal to the amount, it will be reverted, otherwise, an integer overflow will also revert the execution.

uint256 allowed = allowance[holder][msg.sender];
if (allowed >= previewAmount) {
    revert Approvals(allowed, previewAmount);
}
allowance[holder][msg.sender] -= previewAmount;

Recommended Mitigation Steps

• Change the code to:

   uint256 allowed = allowance[holder][msg.sender];
-  if (allowed >= previewAmount) {
+  if (allowed < previewAmount) {
       revert Approvals(allowed, previewAmount);
   }
   allowance[holder][msg.sender] -= previewAmount;

• 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

• GAS
  • 1. Improve logic
  • 2. Avoid unused returns
  • 3. ++i costs less gas compared to i++ or i += 1
  • 4. delete optimization
  • 5. Reduce math operations

GAS

1. Improve logic

Move the following line from VaultTracker.sol#L155-L158:

    Vault memory from = vaults[f];
-   Vault memory to = vaults[t];

    if (a > from.notional) { revert Exception(31, a, from.notional, address(0), address(0)); }
+   Vault memory to = vaults[t];

2. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• Swivel.sol#L103
• Swivel.sol#L422-L679
• MarketPlace.sol#L49-L138
• MarketPlace.sol#L254-L339

3. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• Swivel.sol#L100
• Swivel.sol#L269
• Swivel.sol#L418
• Swivel.sol#L511

4. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

• Swivel.sol#L448
• Swivel.sol#L464
• Swivel.sol#L484
• Swivel.sol#L534
• Swivel.sol#L560

5. Reduce math operations

Use scientific notation (e.g. 10e17) rather than exponentiation (e.g. 10**18)

Change 2**256 - 1 to type(uint256).max:

• Swivel.sol#L549