NO RETURN VALUE CHECK WHILE TRANSFERRING NOTIONAL FEES

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/VaultTracker/VaultTracker.sol#L238 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Marketplace/MarketPlace.sol#L330 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L141 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L233

Vulnerability details

Impact

The implementation of VaultTracker.transferNotionalFee() is used by MarketPlace.transferVaultNotionalFee() which is meant to be used to perform transfers of Notional fees to the Swivel contract without recalculating the interest.

This is a function that allows only calls coming from the Swivel. Precisely, each call that relies on Swivel.initiateVaultFillingVaultExit() and Swivel.initiateVaultFillingZcTokenInitiate() will call afterwards MarketPlace.transferVaultNotionalFee(), and then VaultTracker.transferNotionalFee(). The error that is meant to be reverted may never be triggered because the call will always drag a true return value.

Proof of Concept

The VaultTracker.transferNotionalFee() implementation will always return true if the execution gets Line 238 without assigning the return value to false before that.

Because Marketplace.transferVaultNotionalFee() does not drag the return value of VaultTracker.transferNotionalFee(), its return value will always be true due to MarketPlace Line 330. This also will make that the boolean checks performed within the if statements on Swivel Line 141 and Line 233 will always be true not triggering the intended Exception error.

Recommended Mitigation Steps

As a mitigation for this issue, it is advised to evaluate the scenarios where either VaultTracker.transferNotionalFee() and Marketplace.transferVaultNotionalFee() has to return false. Also, it is advised to check how will the false and true values be dragged from each function return towards the last call in order to have the intended behavior.

[JTraversa]

I'd classify this within QA and a review of unnecessary returns.

[bghughes]

I'd classify this within QA and a review of unnecessary returns.

I believe this should be considered Medium risk as the intended functionality of the checks found here: https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L141 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L233 will not work as expected.

Given that both VaultTracker.transferNotionalFee() and MarketPlace.transferVaultNotionalFee() always return true, the functionality of the if statements (https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L141; https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L233) will never trigger causing the check on fee transfer to potentially be broken.

[robrobbins]

so i don't see any situation where returning false is useful - this would be like saying that Erc20. transfer should return false as well in some sitch

this just returns true or reverts, pretty typical pattern.

we can hoist the bool back toward the callers tho by returning the invocation of the function - that i'll do

[JTraversa]

So the issue would be which revert string is properly returned?

Or is the issue just that the returns themselves arent that useful?

it is advised to evaluate the scenarios where either VaultTracker.transferNotionalFee() and Marketplace.transferVaultNotionalFee() has to return false

In the words of the warden, they dont really know if or what an issue is either, however it most definitely does not impact protocol functionality.

As far as medium severity, its defined as "the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path", so I'd expect the issue would fit one of those boxes, which it does not appear to.

[robrobbins]

imo i simply returned the actual invocation of the vaultTracker method to the calling marketPlace

https://github.com/Swivel-Finance/gost/pull/418

such that -- if not a revert -- that boolean allows the marketplace method to continue on as it should. we do this with various token calls all the time

[robrobbins]

"drag" bool from vaultTracker to swivel: https://github.com/Swivel-Finance/gost/pull/418

[0xean]

downgrading to QA. No leak of value or attack is shown here, so it comes down to code clarity

[0xean]

Marking as duplicate of #127 the wardens QA report.