Swivel v3 contest - Sm4rty's results

1. Missing checks for approve()’s return status

Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin’s safeApprove(), which reverts if there’s a failure, instead

There is 1 instance of this issue: Swivel/Swivel.sol:562

Swivel/Swivel.sol:562:      Safe.approve(uToken, c[i], max);`

Recommendations:

Use OpenZeppelin’s safeApprove()

---

---

2. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances:

Multiple lines in at swivel.sol

Swivel/Swivel.sol:33:  address public aaveAddr; // TODO immutable?
Swivel/Swivel.sol:120:    // TODO cheaper to assign amount here or keep the ADD?
Swivel/Swivel.sol:157:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:192:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:221:    // TODO assign amount or keep ADD?
Swivel/Swivel.sol:286:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:317:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:347:    // TODO assign amount or keep the ADD?
Swivel/Swivel.sol:382:    // TODO assign amount or keep the ADD?
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
Swivel/Swivel.sol:708:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:716:      // TODO explain the Aave deposit args
Swivel/Swivel.sol:721:      // TODO explain the 0 (primary account)
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
Swivel/Swivel.sol:741:    if (p == uint8(Protocols.Compound)) { // TODO is Rari a drop in here?
Swivel/Swivel.sol:748:      // TODO explain the withdraw args
Swivel/Swivel.sol:752:      // TODO explain the 0

---

---

3. Avoid use of floating pragma

Impact

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances

Creator/Erc20.sol Creator/ZcToken.sol Creator/IRedeemer.sol Creator/IERC5095.sol Tokens/Erc20.sol:4 Tokens/ZcToken.sol:2 Tokens/IRedeemer.sol:2 Tokens/IERC5095.sol:2 Marketplace/Erc20.sol:4

Creator/Erc20.sol:4:	pragma solidity ^0.8.0;
Creator/ZcToken.sol:2:	pragma solidity ^0.8.4;
Creator/IRedeemer.sol:2:pragma solidity ^0.8.0;
Creator/IERC5095.sol:2:	pragma solidity ^0.8.0;
Tokens/Erc20.sol:4:	pragma solidity ^0.8.0;
Tokens/ZcToken.sol:2:	pragma solidity ^0.8.4;
Tokens/IRedeemer.sol:2:	pragma solidity ^0.8.0;
Tokens/IERC5095.sol:2:	pragma solidity ^0.8.0;
Marketplace/Erc20.sol:4:pragma solidity ^0.8.0;

Recommended Mitigation Steps

use fixed solidity version

---

---

4. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

Creator/ZcToken.sol:148 Tokens/ZcToken.sol:148

Creator/ZcToken.sol:148:        _mint(t, a);
Tokens/ZcToken.sol:148:        _mint(t, a);

Recommendations:

Use _safeMint() instead of _mint().

1. Use calldata instead of memory

Use calldata instead of memory to save gas. See here for reference: https://code4rena.com/reports/2022-04-badger-citadel/#g-12-stakedcitadeldepositall-use-calldata-instead-of-memory

Instances:

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

Swivel/Swivel.sol:495:  function setFee(uint16[] memory i, uint16[] memory d) external authorized(admin) returns (bool) {

---

---

2. Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances Includes:

Links: Creator/LibFuse.sol:36 VaultTracker/LibFuse.sol:36 Tokens/LibFuse.sol:36

Creator/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
VaultTracker/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");
Tokens/LibFuse.sol:36:            require(borrowRateMantissa <= 0.0005e16, "RATE_TOO_HIGH");

References:

https://blog.soliditylang.org/2021/04/21/custom-errors/

Remediation:

I suggest replacing revert strings with custom errors.

---

---

Preincrement costs less gas as compared to Postincrement :

++i costs less gas as compared to i++ for unsigned integer, as per-increment is cheaper(its about 5 gas per iteration cheaper)

i++ increments i and returns 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 , no need for temporary variable here

In the first case, the compiler has create a temporary variable (when used) for returning 1 instead of 2.

Instances:

Swivel/Swivel.sol:100: Swivel/Swivel.sol:269: Swivel/Swivel.sol:418: Swivel/Swivel.sol:511: Swivel/Swivel.sol:564:

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++;

---

---