code423n4 commented 2 years ago

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.

JTraversa commented 2 years ago

Because we have a feenominator array, with four different feenominator values, we can modulate the fees per order type to ensure that the value of fees is roughly equivalent.

That said, this is a heuristic, and the suggestion could be a way to be a bit more precise, though this is really an implementation detail that surrounds design more than anything (what if we WANT to have certain order types more attractive?)

bghughes commented 2 years ago

I feel that this is a bad issue given that it seems impossible to benefit from fee calculation discrepancies in the context of Swivel.

I disagree with:

The dependency arises because initiateVaultFillingZcTokenInitiate and exitZcTokenFillingZcTokenInitiate pay fees on the same amount but in notional and underlying respectively. due to the consistent use of basing the fee off of principalFilled in the two cases the warden references: https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L296 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L140

It seems to me that the fee is paid off the principal amount in each scenario and I find this warden's argument hard to follow. The one point the warden makes that seems informational and that @JTraversa is aware of is that the inconsistent use of denominator values in this fee math could cause unexpected issues or fee rates for the user.

Downgrading to Informational/QA as I don't think the path the warden described is possible to execute in practice.

bghughes commented 2 years ago

Treating this as the wardens main QA report as they did not submit one - accidentally linked to another warden's QA above and deleted/updated comments to reflect the truth. This will act as 0x52's QA report

robrobbins commented 2 years ago

puttin on a maybe cuz it's all a bit hazy... will circle back

code-423n4 / 2022-07-swivel-findings