admin in contracts is changed through a one step process calling the function setAdmin(). The current admin could inadvertently pass the wrong address to setAdmin() and loose access to permissioned functions. Swivel.sol#L428, MarketPlace.sol#53, Creator.sol#47
3. Redundant Checks on protocol withdrawals and deposits
In Swivel.deposit() and Swivel.withdrawal(), when depositing to or withdrawing from the protocol, the checks on the value returned from the protocol are redundant in most cases. For example in Swivel.sol#L712
return IYearn(c).deposit(a) >= 0;
is equivalent to
return true;
since IYearn.deposit() decodes the returned data as an uint256, so the returned value is always >= 0.
Same for the checks at the following lines, where one could just use return true:
Swivel.sol#L727,
Swivel.sol#L745,
Swivel.sol#L749,
Swivel.sol#L757.
4. Modified implementation of library FixedPointMathLib
function lnWad(int256 x) internal pure returns (int256 r) {
unchecked {
- require(x > 0, "UNDEFINED");
+ if (x < 0) revert Undefined();
.....
function log2(uint256 x) internal pure returns (uint256 r) {
- require(x > 0, "UNDEFINED");
+ if (x < 0) revert Undefined();
This changes the behavior of the functions when x = 0, leading to potential bugs. Even if these functions are not used currently in the protocol, consider implementing the same checks as in the original library and adding a note on the modified file.
1. Missing events for critical changes in system parameters
Change of critical system parameters should come with an event emission to allow for monitoring of potentially dangerous or malicious changes. Swivel.sol#L428, MarketPlace.sol#45, MarketPlace.sol#53, MarketPlace.sol#336, Creator.sol#47, Creator.sol#59
2. One step
admin
changesadmin
in contracts is changed through a one step process calling the functionsetAdmin()
. The currentadmin
could inadvertently pass the wrong address tosetAdmin()
and loose access to permissioned functions. Swivel.sol#L428, MarketPlace.sol#53, Creator.sol#473. Redundant Checks on protocol withdrawals and deposits
In
Swivel.deposit()
andSwivel.withdrawal()
, when depositing to or withdrawing from the protocol, the checks on the value returned from the protocol are redundant in most cases. For example in Swivel.sol#L712is equivalent to
since
IYearn.deposit()
decodes the returned data as anuint256
, so the returned value is always >= 0. Same for the checks at the following lines, where one could just usereturn true
: Swivel.sol#L727, Swivel.sol#L745, Swivel.sol#L749, Swivel.sol#L757.4. Modified implementation of library
FixedPointMathLib
The library
FixedPointMathLib
has been modified (from https://github.com/Rari-Capital/solmate/blob/983e1d65dfb5880c57ff9106c5ed47270a4d6961/src/utils/FixedPointMathLib.sol) to introduce custom errors, but this is not written on the modified file, so an auditor could just assume that this is the standard solmate implementation. Also, input validation checks inlnWad()
andlog2()
have been modified. This can be seen from the diff:This changes the behavior of the functions when
x = 0
, leading to potential bugs. Even if these functions are not used currently in the protocol, consider implementing the same checks as in the original library and adding a note on the modified file.5. Missing natspec parameters
In
ZcToken
, Natspec parameterholder
is missing for functionswithdraw()
(ZcToken.sol#L98) andredeem()
(ZcToken.sol#L124)6. Unreacheable revert statements
In
MarketPlace
andSwivel
most of the checks on return values of external calls do nothing. For example, in MarketPlace.sol#L118:IZcToken.burn()
either reverts or returnstrue
, so the revert statement in the if block is never reached and the custom error is never thrown. The other occurrencies of the same issue are: MarketPlace.sol#L120, MarketPlace.sol#L134, MarketPlace.sol#L136, MarketPlace.sol#L154, MarketPlace.sol#L161, MarketPlace.sol#L136, MarketPlace.sol#L181, MarketPlace.sol#L249, MarketPlace.sol#L251, MarketPlace.sol#L267, MarketPlace.sol#L285, MarketPlace.sol#L287, MarketPlace.sol#L302, MarketPlace.sol#L316, Swivel.sol#L176, Swivel.sol#L205, Swivel.sol#L229, Swivel.sol#L233, Swivel.sol#L301, Swivel.sol#L331, Swivel.sol#L366, Swivel.sol#L399, Swivel.sol#L588, Swivel.sol#L603This worsens off-chain monitoring since the expected custom errors (with specific error codes) are never thrown.