code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L49-L77 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L82-L109 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L113-L139 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L152-L203 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L208-L239

Vulnerability details

Impact

Loss of funds in an underlying protocol would disable most functions of immature markets for that protocol

Proof of Concept

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

Most functions in vaultTracker have the lines of code shown above. If there was ever loss of funds from an underlying protocol, exchangeRate would drop causing the yield calculation for immature markets to underflow and revert. This is unfair as mature positions would still be able to redeem at the face value of their zctokens, while users in immature positions would be totally frozen.

Tools Used

Recommended Mitigation Steps

In each location change the above code to:

if (maturityRate > 0) { yield = ((maturityRate 1e26) / sVault.exchangeRate) - 1e26; } else if (sVault.exchangeRate <= exchangeRate) { yield = ((exchangeRate 1e26) / sVault.exchangeRate) - 1e26; }

This would keep the functions for immature markets from freezing and give both mature and immature positions equal ability to withdraw

JTraversa commented 2 years ago

Unsure if only a severity disagreement or a complete dispute.

Should an integrated market have an issue, we would not allow ANY users to continue to withdraw, but would pause our markets and properly distribute funds.

Doing as the suggestion mentions would still only favor those who can frontrun the hack and withdraw their capital at the cost of others.

So with the context of us having a workflow that can already avoid such issues, and the suggestion itself likely stratifying users in a worse manner than they already would be, I'm not sure what to glean here.

bghughes commented 2 years ago

Unsure if only a severity disagreement or a complete dispute.

Should an integrated market have an issue, we would not allow ANY users to continue to withdraw, but would pause our markets and properly distribute funds.

Doing as the suggestion mentions would still only favor those who can frontrun the hack and withdraw their capital at the cost of others.

So with the context of us having a workflow that can already avoid such issues, and the suggestion itself likely stratifying users in a worse manner than they already would be, I'm not sure what to glean here.

I believe this should be downgraded to a Medium Risk issue as it is a DOS that requires an underlying integration to lose value (possible). Importantly, in this issue Swivel would not lose funds as they may in #71 - it seems the Sponsor would be wise to consider these potential changes in the underlying vault's exchange rate. DeFi has proven time and time again that hanging one's hat on efficient market assumptions (e.g. thinking a vault token is "too big to fail") can be risky business.

JTraversa commented 2 years ago

Honestly I dont think this should be accepted. And at most we can only acknowledge it and the same can be said of every yield integrating protocol.

Highlighting my final comment here:

With the context of us having a workflow that can already avoid such issues, and the suggestion itself likely stratifying users in a worse manner than they already would be, I'm not sure what to glean here.

The amelioration seems to leave the protocol in a worse condition than it currently is. It would mean that certain users are favored over others, favoring FIFO withdrawal transactions, rather than preventing withdrawals caused by underflow and proportional distribution.

With that context I'd ask the judge or warden to explain how this is more advantageous for users than the current implementation, or generally help explain what we could glean from this report :/

0xean commented 2 years ago

I think the warden highlights a possible scenario in which the protocol functionality does not behave as expected leading to transactions to revert that otherwise may be able to proceed. If the underlying protocols lose all funds this is somewhat a moot point.

I do believe that the warden highlighting this is beneficial even if the sponsors choose not to fix it since they believe it to be a feature and not a bug.

I am going to downgrade to QA