Swivel v3 contest - Kaiziron's results

Unspecific compiler version pragma

Information : L003 - Unspecific Compiler Version Pragma Consensys Audit of 1inch Solidity docs

Instances include :

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

Recommendation

It is recommended to use a concrete compiler version, for example :

VaultTracker/FixedPointMathLib.sol:2:pragma solidity 0.8.0;

Unsafe ERC20 Operation(s)

Information : L001 - Unsafe ERC20 Operation(s)

Instances include :

Swivel/Swivel.sol:359:    Safe.transfer(uToken, o.maker, a - premiumFilled);
Swivel/Swivel.sol:363:    Safe.transfer(uToken, msg.sender, premiumFilled - fee);
Swivel/Swivel.sol:395:    Safe.transfer(uToken, msg.sender, principalFilled - a - fee);
Swivel/Swivel.sol:396:    Safe.transfer(uToken, o.maker, a);
Swivel/Swivel.sol:467:    Safe.transfer(token, admin, token.balanceOf(address(this)));
Swivel/Swivel.sol:609:    Safe.transfer(IErc20(u), msg.sender, a);
Swivel/Swivel.sol:624:    Safe.transfer(IErc20(u), t, a);
Swivel/Swivel.sol:642:    Safe.transfer(IErc20(u), msg.sender, redeemed);
Swivel/Swivel.sol:661:    Safe.transfer(IErc20(u), msg.sender, redeemed);
Swivel/Swivel.sol:125:    Safe.transferFrom(uToken, msg.sender, o.maker, a);
Swivel/Swivel.sol:128:    Safe.transferFrom(uToken, o.maker, address(this), principalFilled);
Swivel/Swivel.sol:163:    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
Swivel/Swivel.sol:167:    Safe.transferFrom(uToken, msg.sender, address(this), (a + fee));
Swivel/Swivel.sol:199:    Safe.transferFrom(uToken, msg.sender, o.maker, a - premiumFilled);
Swivel/Swivel.sol:202:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:224:    Safe.transferFrom(IErc20(o.underlying), msg.sender, o.maker, a);
Swivel/Swivel.sol:293:    Safe.transferFrom(uToken, o.maker, msg.sender, principalFilled - a);
Swivel/Swivel.sol:298:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:324:    Safe.transferFrom(uToken, o.maker, msg.sender, premiumFilled);
Swivel/Swivel.sol:328:    Safe.transferFrom(uToken, msg.sender, address(this), fee);
Swivel/Swivel.sol:580:    Safe.transferFrom(uToken, msg.sender, address(this), a);

Recommendation

It is recommended to always use OpenZeppelin's SafeERC20 library, for example :

Swivel/Swivel.sol:359:    Safe.safeTransfer(uToken, o.maker, a - premiumFilled);

Use two-step transfer pattern for access controls

Contracts that implement access control, e.g. admin, should consider implementing a two-step transfer pattern. Otherwise, it is possible that the role mistakenly transfers ownership to the wrong address, resulting in losing the role.

Instances include :

https://github.com/code-423n4/2022-07-swivel/blob/main/Marketplace/MarketPlace.sol#L53-L56 https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Swivel.sol#L428-L432 https://github.com/code-423n4/2022-07-swivel/blob/main/Creator/Creator.sol#L47-L50

Recommendation

It is recommended separate the ownership transfer into 2 functions, first to set a pending owner, and second to approve the pending admin set using the first function and actually implementing the change of the ownership.

If possible, use prefix increment instead of postfix increment

Prefix increment ++i returns the updated value after it's incremented and postfix increment i++ returns the original value then increments it. Prefix increment costs less gas compared to postfix increment.

Instances includes :

Marketplace/Erc20.sol:154:                                nonces[owner]++,
Swivel/Swivel.sol:100:      unchecked {i++;}
Swivel/Swivel.sol:269:      unchecked {i++;}
Swivel/Swivel.sol:418:        i++;
Swivel/Swivel.sol:511:        x++;
Swivel/Swivel.sol:564:        i++;
Creator/Erc20.sol:154:                                nonces[owner]++,

Recommendation

It is recommended to use prefix increment instead of postfix one when the return value is not needed, as both of them will give the same result and prefix increment costs less gas.

For example :

Swivel/Swivel.sol:418:        ++i;