code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

With most functions in VaultTracker.sol, users can call them only once after maturity has been reached.

So from the second call of any functions after maturity, it will revert and users might lose their funds or interests.

Proof of Concept

The main problem is that vlt.exchangeRate might be larger than maturityRate after maturity here.

Then it will revert from the second call with uint underflow here.

So such scenario is possible.

Alice splits 1000 USDC into zcToken and nToken using splitUnderlying()
After the market is martured, she withdraws back 1000 USDC using combineTokens()
While combining tokens, removeNotional() is called and vlt.exchangeRate will be set as an exchangeRate after maturity here.
From the explanation about Exchange Rate, we know this ratio increases over time and also exchangeRate after the market is matured is greater than market.maturityRate from this function for zcToken interest
So after removeNotional() is called, vlt.exchangeRate > maturityRate.
After that, Alice calls redeemVaultInterest() to claim interest, but it will revert here with uint underflow error because maturityRate < vlt.exchangeRate.
So she can't claim her interest forever.
Also, if she claims interest first before calling combineTokens(), she can't get paid back here 1000 USDC forever because removeNotional() will revert here for the same reason.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We should save vlt.exchangeRate = maturityRate when maturityRate > 0 and exchangeRate > maturityRate.

vlt.exchangeRate = (maturityRate > 0 && maturityRate < exchangeRate) ? maturityRate : exchangeRate;

There are several places to modify for safety.

JTraversa commented 2 years ago

I dont actually disagree with most of the warden's claims, just note that all of this would need to be intentional on the part of the user, and could not happen under any normal workflow of the protocol.

E.g.:

Alice splits 1000 USDC into zcToken and nToken using splitUnderlying() After the market is martured, she withdraws back 1000 USDC using combineTokens()

This interaction relies on a user calling combineTokens past maturity, which is 1. not possible from our UI 2. not beneficial in any way whatsoever, as redemption is explicitly done post maturity through redeemZcToken, + not using redeemZcToken would also fail to accrue post maturity interest.

The same can be said of:

Also, if she claims interest first before calling combineTokens(), she can't get paid back here 1000 USDC forever because removeNotional() will revert here for the same reason.

There is no use case to calling combineTokens after maturity, meaning this specifically isn't an issue in that direction either.

Just disagreeing with severity because of the lack of potential impact in any reasonable operation of the protocol. A user would have to do this through a script, and the workflow would be unlikely to happen on accident.

robrobbins commented 2 years ago

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

0xean commented 2 years ago

I am somewhere between leaving this as high and downgrading to medium. I understand the sponsors points, but think its a pretty nasty rough edge to leave out there if a user interacted with the contract through etherscan or some other UI with less guard rails.

I am going to downgrade to medium based on it being somewhat self inflicted and the probability of it therefore very low.

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