QA Report

[code423n4]

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().

[robrobbins]

on use safe.

we do

on safeMint

this is NFT specific, doesn't apply here.