Swivel v3 contest - Bahurum's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L112 https://github.com/code-423n4/2022-07-swivel/blob/fd36ce96b46943026cb2dfcb76dfa3f884f51c18/Creator/ZcToken.sol#L133

Vulnerability details

Title

Cannot withdraw or redeem approved tokens

Impact

A contract/EOA which has been approved some ZcToken cannot redeem() or withdraw() the approved tokens since these functions always revert if msg.sender != holder.

Proof of Concept

In the withdraw() function (ZcToken.sol#L112) the check on allowance

            if (allowed >= previewAmount) {
                revert Approvals(allowed, previewAmount);
            }

is incorrect. So the withdraw() always reverts if msg.sender != holder, since if allowed >= previewAmount, then it reverts and otherwise allowance[holder][msg.sender] -= previewAmount; it reverts for underflow.

In the redeem() function the issue is similiar (ZcToken.sol#L133) but concerns the check allowed >= principalAmount which is incorrect.

Tools Used

Manual analysis

Recommended Mitigation Steps

Use allowed < previewAmount (in withdraw()) and allowed < principalAmount (in redeem()) to check amount exceding allowance.

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 changes

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

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 in lnWad() and log2() have been modified. This can be seen from the diff:

     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.

5. Missing natspec parameters

In ZcToken, Natspec parameter holder is missing for functions withdraw() (ZcToken.sol#L98) and redeem() (ZcToken.sol#L124)

6. Unreacheable revert statements

In MarketPlace and Swivel most of the checks on return values of external calls do nothing. For example, in MarketPlace.sol#L118:

    if (!IZcToken(market.zcToken).burn(t, a)) { revert Exception(29, 0, 0, address(0), address(0)); }

IZcToken.burn() either reverts or returns true, 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#L603

This worsens off-chain monitoring since the expected custom errors (with specific error codes) are never thrown.