Y2k Finance contest - Tointer's results

Judge has assessed an item in Issue #149 as High risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L215-L218 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L116-L119

Vulnerability details

Impact

Any third-party integration for vault tokens create attack vector that would result in user funds lost

Premise for attack:

1. User should call setApprovalForAll for some smart contract. It's not necessary for this contract to be malicious. Few examples of such contracts:
   • Smart contract that would withdraw funds on behalf of user and then deposit it in next epoch in one tx for better UX
   • Uniswap pool
   • Trading platforms like Seaport and Zora
   • Lending platforms
2. User should deposit to some vault and wait until epoch ends

Attack:

Anyone could call withdraw function and withdraw funds to smart contract address. They would be lost, since smart contract cannot possible have functionality to process this withdraw

Why I think this premise is valid:

User could easily call setApprovalForAll for some contract if they see that it's code is not malicious. They would not be expecting that invoking this function is equal to whitelisting address to withdrawal that could be called by anyone, like we not expecting that someone could send our tokens to Uniswap pool contract after we use it.

Recommended Mitigation Steps

receiver should always be msg.sender. Good example is how OpenZeppelin do it: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7a14f6c5953a1f2228280e6eb1dfee8e5c28d79a/contracts/token/ERC1155/ERC1155.sol#L125

getNextEpoch could become unusable

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L438-L452 In the long run, this function could become unusable, as it would consume too much gas. This could be fixed with binary search algorithm

epochHasEnded modifier not working as intended

If someone would call withdraw after epoch expires but before triggerEndEpoch call, modifier epochHasEnded would not work as intended and would not revert. Withdraw would revert on beforeWithdraw when it would try to divide by idFinalTVL which is 0. Since it used only in withdraw, there is no risk. Solution: add idFinalTVL check in epochHasEnded modifier https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L96

Redundant token transfer

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L168 this line can be removed, right after it tokens transfered other way

eip-4626-like functionality is broken

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L244-L252 Here, totalAssets always return number of shares, instead of underlying asset. This renders this functions https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L143-L228 useless, as shares would always be equal to assets. eip-4626 deposit and withdraw functions are not used either, since they get overriden in Vault.sol contract. Solution: remove ERC4626 vault functionality alltogether to greatly simplify codebase.