Swivel v3 contest - slywaters's results

Low Severity Findings

Check addresses when using solmate's safeTransferLib

This project makes use of a modified version of a solmate library. The safeTransferLib library does not perform verification on addresses, specifically whether a token address actually contains code. This means that misuse of this function could result in assets being transferred to incompatible accounts. When this occurs, the transaction will not revert and the function call will not return false.

It is advised to manually verify function arguments when using this library.

Reference:

• Solmate developer warning

The following lines of code are affected. The code proceeding these lines should be modified to include checks before these function calls:

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

QA

Developer notes should be removed from production code

Comments in production code should not contain developer discussion or notes about known bugs or problems. These issues should be tracked elsewhere and resolved before being deployed.

Comments of this kind can also indicate potential avenues of attack for an adversary.

The following lines are affected:

2022-07-swivel/Swivel/Swivel.sol:120:    // TODO cheaper to assign amount here or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:157:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:192:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:221:    // TODO assign amount or keep ADD?
2022-07-swivel/Swivel/Swivel.sol:286:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:317:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
2022-07-swivel/Swivel/Swivel.sol:347:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:382:    // TODO assign amount or keep the ADD?
2022-07-swivel/Swivel/Swivel.sol:707:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
2022-07-swivel/Swivel/Swivel.sol:716:      // TODO explain the Aave deposit args
2022-07-swivel/Swivel/Swivel.sol:721:      // TODO explain the 0 (primary account)
2022-07-swivel/Swivel/Swivel.sol:740:    // TODO as stated elsewhere, we may choose to simply return true in all and not attempt to measure against any expected return
2022-07-swivel/Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
2022-07-swivel/Swivel/Swivel.sol:748:      // TODO explain the withdraw args
2022-07-swivel/Swivel/Swivel.sol:752:      // TODO explain the 0

Gas Report

Replace postfix increment (var++) with prefix increment (++var)

Using ++i costs less gas than using i++. In the context of a for-loop, gas is saved on each iteration.

The following lines of code are affected:

2022-07-swivel/Swivel/Swivel.sol:418:        i++;
2022-07-swivel/Swivel/Swivel.sol:511:        x++;
2022-07-swivel/Swivel/Swivel.sol:564:        i++;

"calldata" can be used instead of "memory" to reduce gas costs

Both memory and calldata allow a developer to manage variables within the scope of a function. memory is mutable which makes it a more flexible tool; however, it costs additional gas when compared with calldata. calldata is non-modifiable but cheaper. Therefore it is recommended to use calldata for function parameters when they will not be modified.

For more information, see the following resources: Solidity Documentation recommending the use of calldata

Solidity — Storage vs Memory vs Calldata

The following function calls can be changed to use calldata instead of memory:

2022-07-swivel/Swivel/Hash.sol:59:  function domain(string memory n, string memory version, uint256 i, address verifier) internal pure returns (bytes32) {
2022-07-swivel/Swivel/Sig.sol:38:  function splitAndRecover(bytes32 h, bytes memory sig) internal pure returns (address) {
2022-07-swivel/Swivel/Sig.sol:48:  function split(bytes memory sig) internal pure returns (uint8, bytes32, bytes32) {
2022-07-swivel/Swivel/Swivel.sol:495:  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {

Use strict less-than/greather-than comparisons rather than less-than-or-equal/greater-than-or-equal

When compiled, Solidity code using the >= or <= comparison operators in fact executes two separate checks: one for 'is-equal-to' and a second for 'is-greater-than/is-less-than'. By contrast, using > or < performs only one check. Therefore code that is written to use strict comparison operators is more gas-efficient.

If this change is applied, be sure to update the relevant variables being evaluated. For clarity, it is also advised to rename the variables to make this change explicit, e.g. renaming a variable from MINIMUM to MINIMUM_PLUS_ONE.

The following lines are affected:

2022-07-swivel/Creator/FixedPointMathLib.sol:47:            if (x <= -42139678854452767551) return 0;
2022-07-swivel/Creator/FixedPointMathLib.sol:51:            if (x >= 135305999368893231589) revert ExpOverflow();
2022-07-swivel/Creator/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
2022-07-swivel/Creator/ZcToken.sol:112:            if (allowed >= previewAmount) {
2022-07-swivel/Creator/ZcToken.sol:133:            if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
2022-07-swivel/Marketplace/FixedPointMathLib.sol:47:            if (x <= -42139678854452767551) return 0;
2022-07-swivel/Marketplace/FixedPointMathLib.sol:51:            if (x >= 135305999368893231589) revert ExpOverflow();
2022-07-swivel/Marketplace/LibFuse.sol:39:            if(borrowRateMantissa <= 0.0005e16) { revert RATE(); }
2022-07-swivel/Swivel/Swivel.sol:712:      return IYearn(c).deposit(a) >= 0;
2022-07-swivel/Swivel/Swivel.sol:727:      return IErc4626(c).deposit(a, address(this)) >= 0;
2022-07-swivel/Swivel/Swivel.sol:745:      return IYearn(c).withdraw(a) >= 0;
2022-07-swivel/Swivel/Swivel.sol:749:      return IAave(aaveAddr).withdraw(u, a, address(this)) >= 0;
2022-07-swivel/Swivel/Swivel.sol:757:      return IErc4626(c).withdraw(a, address(this), address(this)) >= 0;
2022-07-swivel/Tokens/FixedPointMathLib.sol:47:            if (x <= -42139678854452767551) return 0;
2022-07-swivel/Tokens/FixedPointMathLib.sol:51:            if (x >= 135305999368893231589) revert ExpOverflow();
2022-07-swivel/Tokens/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
2022-07-swivel/Tokens/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
2022-07-swivel/Tokens/ZcToken.sol:133:            if (allowed >= principalAmount) { revert Approvals(allowed, principalAmount); }
2022-07-swivel/VaultTracker/FixedPointMathLib.sol:46:            // x <= floor(log(0.5e18) * 1e18) ~ -42e18
2022-07-swivel/VaultTracker/FixedPointMathLib.sol:50:            // int. This happens when x >= floor(log((2**255 - 1) / 1e18) * 1e18) ~ 135.
2022-07-swivel/VaultTracker/FixedPointMathLib.sol:51:            if (x >= 135305999368893231589) revert ExpOverflow();
2022-07-swivel/VaultTracker/LibCompound.sol:28:        require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH"); // Same as borrowRateMaxMantissa in CTokenInterfaces.sol
2022-07-swivel/VaultTracker/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");

Pragma statement should be at least 0.8.4

The pragma declaration allows for Solidity versions less than version 0.8.4. Several gas optimization features have been introduced in versions of Solidity between 0.8.0 and 0.8.4, including:

• The "low-level inliner" (version 0.8.2)
• Improvements to the optimizer (version 0.8.3)
• Custom errors (version 0.8.4)

Please note that Solidity version 0.8.9 contains bugfixes in addition to these gas improvements and that if possible it is advised to use versions greater than or equal to 0.8.9 for additional benefit.

For more information consult the following resources:

• Saving Gas with Simple Inlining
• Solidity 0.8.3 release announcement: Optimizer improvements
• Solidity docs: errors and the revert statement

The following pragma statements should be updated:

2022-07-swivel/Creator/Erc20.sol:4:pragma solidity ^0.8.0;
2022-07-swivel/Creator/IERC5095.sol:2:pragma solidity ^0.8.0;
2022-07-swivel/Creator/IRedeemer.sol:2:pragma solidity ^0.8.0;
2022-07-swivel/Marketplace/Erc20.sol:4:pragma solidity ^0.8.0;
2022-07-swivel/Tokens/Erc20.sol:4:pragma solidity ^0.8.0;
2022-07-swivel/Tokens/IERC5095.sol:2:pragma solidity ^0.8.0;
2022-07-swivel/Tokens/IRedeemer.sol:2:pragma solidity ^0.8.0;