QA Report

[code423n4]

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

[JTraversa]

low hanging "TODO" that may be there in future versions, normally considerations / acknowledgements. It gives people a view into our current thought processes.