Swivel v3 contest - scaraven's results

Lines of code

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

Vulnerability details

Impact

As defined in the docs for Euler, ERC4626, Compound and Aave, when withdrawing and depositing funds the amount specified corresponds excactly to how many of the underlying assets are deposited or withdrawn.

However, as specified by Yearn, the yearn withdraw amount parameter specifies how many shares are burnt instead of underlying assets retrieved. Two scenarios can occur from then on, if there are not enough shares then the transaction will revert and users will not be able to redeem the underlying assets from Yearn. If there are enough shares, then a higher number of assets will be withdrawn then expected (as assets per share will only increase). This means that once the user receives their underlying assets at a 1:1 peg the excess amoutns will be permanently locked in the vault contract.

Proof of Concept

All scenarios use Yearn finance as the protocol, the other protocols are unaffected

Scenario #1

1. A yearn finance vault contains 50 shares and 200 uTokens all owned by Swivel.sol
2. A user opens a zcToken position and deposits 100 uTokens into yearn receiving 100 zcTokens (and possibly a premium)
3. 100 / 200 * 50 = 25 shares are minted for the vault so we now have 75 shares and 300 uTokens in the yearn vault
4. After maturity is achieved, the user tries to redeem their 100 zcTokens for uTokens
5. The swivel.sol contract tries to withdraw the 100 uTokens, it instead withdraws 100 shares which is less than available so the transaction reverts and the user cannot withdraw all of their 100 uTokens and instead can only withdraw 75 uTokens.

Scenario #2

1. A yearn finance vault contains 50 shares and 100 uTokens all owned by Swivel.sol
2. A user opens a zcToken position and deposits 100 uTokens into yearn receiving 100 zcTokens (and possibly a premium)
3. 100 / 100 * 50 = 50 shares are minted for the vault so we now have 100 shares and 200 uTokens in the yearn vault
4. After maturity is achieved, the user tries to redeem their 100 zcTokens for uTokens
5. The contract tries to retrieve 100 uTokens but instead withdraws 100 shares which corresponds to 200 uTokens
6. User receives their 100 uTokens causing 100 uTokens to be left in the Swivel.sol contract which are now irretrievable
7. If any user tries to withdraw their funds then the transaction will fail as no shares are left in the yearn vault

Tools Used

VS Code

Recommended Mitigation Steps

In the withdraw() function in Swivel.sol, calculating the price per share and use that to retrieve the correct number of underlying assets e.g.

uint256 pricePerShare = IYearnVault(c).pricePerShare();
return IYearnVault(c).withdraw(a / pricePerShare) >= 0;

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L148-L167 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/Interfaces.sol#L52 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L620

Vulnerability details

Impact

authRedeem() in Marketplace.sol uses the ISwivel interface and calls authRedeem() at Swivel.sol instead of authRedeemZcToken which does not exist. Therefore any transaction where a user tries to redeem their zcTokens for uTokens will fail.

Proof of Concept

1. A user opens a zcToken position by depositing 100 uTokens
2. After maturity, the user tries to redeem their zcTokens using zcToken.withdraw()
3. marketplace.authRedeem() is called which then calls ISwivel(swivel).authRedeem()
4. This function however does not exist so the transaction reverts and prevents the user from withdrawing their uTokens

Tools Used

VS Code

Recommended Mitigation Steps

Change authRedeemZcToken() in Swivel.sol to authRedeem()

Issue #1

Compounding.sol implements underlyingAssets() and exchangeRate() for Rari Fuse. It however does not allow for withdrawals or deposits in Swivel.sol. Therefore a user can create a market for Rari but is barred from depositing or withdrawing funds making this functionality obselete

Consider adding the withdraw() and deposit() abstractions for RariFuse or removing it alltogether

Occurences: https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/Compounding.sol#L44 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Tokens/Compounding.sol#L44 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/Compounding.sol#L54 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/Compounding.sol#L72 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Creator/Compounding.sol#L44