Swivel v3 contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L113-L139

Vulnerability details

Impact

Loss of all user funds

Proof of Concept

This exploit stems from a quirk in the way that exchange rate is tracked for matured positions. We first need to breakdown how interest is calculate for a matured position.

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L123-L132

In L124 the yield for a matured position is calculated as the difference between the previous exchange ratio and the maturity rate. This counts on the fact the exchange rate of the underlying protocol never decreases, as it always set the previous exchange rate to the current exchange rate after yield is calculated regardless of whether it is mature or not. The assumption is that the current exchange rate will always be greater than or equal to the maturity exchange rate. If it is higher then L124 will revert due to underflow and if it is equal then L124 will return yield = 0. The issue comes in when this assumption is invalidated. This would happen were the underlying protocol to lose value (hacked, UST depeg, etc.). With the loss of value, the exchange rate would drop, allowing a user to repeatedly redeem their matured position until all funds from the affected protocol are drained.

Example: Imagine a yearn vault that takes USDC as the underlying token. It's current price per share is 1.25e6 (1.25 USDC per vault share). Swivel has a recently expired VaultTracker.sol for this yearn vault for which mature() is called, setting maturityRate = 1.25e6. Now let's imagine that one small strategy deployed by the vault is compromised and the vault loses 4% of it's USDC. Now the vault will return 1.2e6 as the price per share. When calling redeemInterest for the first time, vlt.exchangeRate will be updated to 1.2e6 in L132. The next time redeemInterest is called it will pay the difference between 1.25e6 (maturityRate) and 1.2e6 (vlt.exchangeRate). redeemInterest can be called repeatedly like this until all USDC deposited in the yearn vault by Swivel users has been drained.

Additionally the user in question would not even need to have an expired position before the loss of funds occurred. SplitUnderlying in Swivel.sol has no checks to keep a user from minting previously expired market. After the loss of funds occurred the malicious user could use SplitUnderlying to create nTokens for the expired market, then carry out the exploit

Tools Used

Recommended Mitigation Steps

The impact of such an event could be decreased with changes. In splitUnderlying add:

require(block.timestamp < m)

This prevents nTokens from being created after expiration which dramatically reduces the ability to take advantage of the opportunity. As for redeemInterest, add the following line after L124:

vlt.notional = 0

This would clear the notional balance of the user when redeeming after maturity, making it impossible to call repeatedly and reduces the chances that any users have a notional balance to exploit it should an event like this happen.

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L148-L168

Vulnerability details

Impact

Complete loss of user funds

Proof of Concept

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L156 In L156 and L164 marketplace.sol makes an external call to swivel.authRedeem, but Swivel.sol doesn't contain any function by that name. When calling a nonexistent function in solidity, the call will simply return immediately with no code execution. This will burn the users tokens and not send them any underlying tokens. It seems that it meant to call authRedeemZcToken instead.

Tools Used

Recommended Mitigation Steps

Change L156 and L164 to: ISwivel(swivel).authRedeemZcToken(p, u, market.cTokenAddr, t, a);

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L111-L144 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L215-L236

Vulnerability details

Impact

User avoids protocol fees

Proof of Concept

The dependency arises because initiateVaultFillingZcTokenInitiate and exitZcTokenFillingZcTokenInitiate pay fees on the same amount but in notional and underlying respectively. Notional will likely carry a much lower value per unit because it represent the future yield on the underlying. Since it is relative we will assume for this example that each notional is worth 0.05e6 (0.05 USDC). This is based on 10% APR and 6 months from maturity (generous but reasonable).

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L127

principalFilled is calculated the same in both functions and the order would be the same regardless of function therefore we will arbitrarily set principalFilled to 1000e6 (1000 USDC).

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L140

initiateVaultFillingZcTokenInitiate use feenominator[2] which is set at 400 in the constructor.

fee = 1000e6 / 400 = 2.5e6

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L141

This fee is transferred as notional so we need to convert the fee paid into USDC for proper comparison.

2.5e6 * 0.05e6 / 1e6 = 0.125e6 (0.125 USDC)

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L296

exitZcTokenFillingZcTokenInitiate uses feenominator[1] which is set at 600 in the constructor

fee = 1000e6 / 600 = 1.66e6

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L298

This fee is transferred in USDC (underlying)

Using initiateVaultFillingZcTokenInitiate results in a fee that is over 10x cheaper (0.125 vs 1.66) compared to exitZcTokenFillingZcTokenInitiate.

The end result isn't completely the same though because in one case, the taker's position was closed and in the other the taker now has two positions instead of one (their initial ntoken position and the newly minted zCToken position), but this is easily resolved. Assume the taker has a position of 1000e6 nTokens that they wish to close. exitZcTokenFillingZcTokenInitiate would allow them to directly close their nToken position with the order. If they instead decided to use initiateVaultFillingZcTokenInitiate with the same order would result in the taker with a new 1000e6 zCToken position. This would perfectly match with the number of nTokens they wish to close. The taker could then call combineTokens to combine their nTokens and the newly minted zCTokens to exit their position. Using initiateVaultFillingZcTokenInitiate and combineTokens together gives the same end result as exitZcTokenFillingZcTokenInitiate while paying a fraction of the fee as demostrated above.

A similar exploit is also possible using initiateVaultFillingVaultExit and splitUnderlying instead of exitZcTokenFillingVaultExit.

Tools Used

Recommended Mitigation Steps

The easiest solution would be to charge the fees for initiateVaultFillingZcTokenInitiate and initiateVaultFillingVaultExit in the underlying. If it is more desirable to charge the fee in notional, then the amount paid for each notional could be found by dividing the amount paid (a) by the amount of notional minted (principalFilled). The fee could then be converted into notional using this conversion rate and then charged as notional.