kartoonjoy
commented
2 years ago Per warden, this issue is to be withdrawn from consideration. Thanks!
Closed code423n4 closed 2 years ago
kartoonjoy
commented
2 years ago Per warden, this issue is to be withdrawn from consideration. Thanks!
Lines of code
https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L208-L239
Vulnerability details
Impact
initiateVaultFillingZcTokenInitiate and initiateVaultFillingVaultExit may become nonfunctional after vault maturity
Proof of Concept
The root of the issue is in VaultTracker.sol transferNotionalFee() L222-231:
https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L222-L231
L224 only functions under the assumption that exchangeRate will never increase after maturity, and will revert if sVault.exchangeRate is ever higher than maturityRate because (maturityRate * 1e26) / sVault.exchangeRate) will become less than 1e26 causing an underflow. Even after maturity, sVault.exchangeRate is updated to the exchangeRate fetched from compounding in L231. If compounding.exchangeRate were to ever return higher than maturityRate then sVault.exchangeRate would be updated to the higher rate and all subsequent transactions would revert at L224. Since swivel adds support for any generic ERC4626 there is no way to guarantee that the exchange rate will never increase after maturity.
Once this were to happen all functions dependent on transferNotionalFee() would always revert. This includes initiateVaultFillingZcTokenInitiate and initiateVaultFillingVaultExit which are dependent via calls to transferVaultNotionalFee in MarketPlace.sol in L141 and L233.
Tools Used
Recommended Mitigation Steps
Regardless of maturity status, the current exchange rate should always be used. Remove maturity check by changing L222-L227 of VaultTracker.sol to:
yield = ((exchangeRate * 1e26) / sVault.exchangeRate) - 1e26;