Mismatch in `withdraw()` between Yearn and other protocols can prevent Users from redeeming zcTokens and permanently lock funds

[code423n4]

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;

[JTraversa]

Duplicate of #30

[scaraven]

I do not understand how this is a duplicate of #30, #30 talks about a problem with redeemUnderlying() in Compound while this issue talks about a problem with withdraw() when using yearn

[JTraversa]

They both cannot be true at once however. Either the lib expects shares, or the lib expects assets. His suggestion notes inconsistency and recommends changing the compound redeem to align with yearn, while you note the inconsistency and recommend fixing the yearn math. At the end of the day the issue is the "mismatch".

[scaraven]

Fair enough, that makes sense. I would still disagree with their interpretation of the issue, it is clear from the code that a represents the underlying assets. If a does represent the number of shares then other functions such as authRedeemzcTokens would be plain wrong because it would be redeeming each zcToken as if it was one share not one underlying asset.

[JTraversa]

Yeah I actually kind of agree with you. We intended the implementation to align more with your ticket.

That said it might be worth a judge's input.

If they think theyre different, the other ticket is invalid/should be disputed and this one is correct.

[bghughes]

Confirmed good issue and mark #30 as a duplicate

[robrobbins]

addressed: https://github.com/Swivel-Finance/gost/pull/437