Lack of checks on `deposit()` and `withdraw()` return for ERC4626 can prevents users from withdrawing funds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L727 https://github.com/code-423n4/2022-07-swivel/blob/daf72892d8a8d6eaa43b9e7d1924ccb0e612ee3c/Swivel/Swivel.sol#L757

Vulnerability details

Impact

As described in the security considerations of the EIP-4626 page, deposit/withdraw may be exposed to slippage, unexpected deposit/withdrawal limits or other features such as fee-on-transfer prevalent in ERC20

This issue stems mainly from the vagueness of ERC4626, allowing implementators to code very different vaults. I believe it is not realistic to expect all contracts to follow solmate's implementation. The other protocols whose code is known do not suffer from such problems.

Currently, deposit() and withdraw() for ERC4626 in Swivel.sol, do not implement any checks for slippage or an incorrect amount being returned so if in the worst case scenario, no assets are withdrawn on withdraw(), the transaction will continue.

For example, if a vault uses a fee-on-deposit/withdraw mechanism then it can result in less assets being received then was expected. However, as users will always receive their expected amount funds irrespective of whether enough was withdrawn, it can prevent other users from being able to redeem tokens from the Swivel.sol contract.

I believe this problem is medium severity because this issue does require external circumstances (a user uses a new ERC4626 implementation) but if this does occur then it can major loss of funds for users.

Proof of Concept

1. A user initiates a zcToken position with 100 uTokens (underlying asset) and receives 100 zcTokens
2. The 100 uTokens are deposited into an ERC4626 contract
3. After maturity, the user tries to redeem their 100 zcTokens by exiting their zcToken position
4. Due to certain reasons (fees, slippage, contract has been paused, etc...) then the swivel.sol contract may receive less uTokens than expected. Lets assume the contract receives 50 uTokens
5. 100 uTokens are transferred to the user however the balance of Swivel.sol is too low so the transaction fails and the user cannot redeem all of their tokens.

If the ERC4626 does experience slippage then no matter what the user tries to withdraw, the contract will always receive less than expected but always tries to send the full amount to the user thereby preventing a user from withdrawing any funds at all.

Tools Used

VS Code

Recommended Mitigation Steps

Consider adding a slippage check for deposit() and transfer the amount of underlying tokens actually withdrawn from the ERC4626 contract instead of the expected amount e.g.

uint256 beforeBalance = IErc20(a).balanceOf(address(this));
IErc4626(c).withdraw(a, address(this));
uint256 assetIncrease = IErc20(a).balanceOf(address(this)) - beforeBalance;
return assetIncrease >= minAmountOut;

Additionally, I would highly suggest that the deposit() and withdraw() functions return the actual amounts deposited or withdrawn so that only the current user is affected by slippage, fees, etc... instead of all other users. This however may not be desirable as it can clash with the current error system being used.

[JTraversa]

Our planned implementation does not include vaults that may have slippage.

ERC4626 was phrased as neutrally as possible to include LP positions on places like Uniswap-v3 but in our case none of that is relevant.

[bghughes]

Warden, I do not think this is a good issue given that the protocol is not concerned about slippage or execution rate in the instances you point out. Note the manner in which these functions operate:

Each returns a boolean simply to denote whether or not the deposit was successful. Therefore the exact execution of a potential 4626 deposit is irrelevant in the scope of the protocol as Swivel will still attempt to unpack those vault tokens and try to provide a ZCB + yield token on top of it.

It is worth noting @JTraversa that allowing for a generic ERC4626 vault like this could cause unexpected behaviors and the team should be prepared to handle any issues that may arise with this; namely, users attempting to use non-"supported" vault tokens or even malicious tokens because you allow them to.

Downgrading to informational given all of the above, no loss of funds, and generally no impacted functionality in practice (users wanting to use fee-on-transfer token vaults is an edge case). Please let me know warden or sponsor if you have any questions about my reasoning here 😄

[bghughes]

Grouping this with the warden’s QA report, #170