Y2k Finance contest - Haruxe's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L203 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L110

Vulnerability details

Impact

In both Vault.sol#L203 and SemiFungibleVault.sol#L110 ( where the original virtual function comes from ) the withdraw() function is used to withdraw entitled deposited assets. The issue lies in the fact that the caller can specify the owner and receiver as long as isApprovedForAll(owner, receiver) passes. This opens up the opportunity for a bad actor to grief by calling the withdraw() function on behalf of a previously approved address and send the funds whenever they like.

Proof of Concept

If a user were to give exclusive access of their token(s) to a contract or another user, a bad actor could call the withdraw() function immediately with their respective addresses which would carry on the transaction, burning the user's tokens, giving fees to the treasury, and giving them to the other party.

Tools Used

VS Code

Recommended Mitigation Steps

Outside users should not have access to others' assets, and msg.sender should be checked again in the function to prove that the caller has the permission to call withdraw() on behalf of another user. If the situation allows, limiting withdraws to msg.sender would mitigate this risk.

It can be noted as well that in the ERC1155.sol contract, the safeTransferFrom() function requires the operator value to be msg.sender().