Y2k Finance contest - KIntern_NA's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L215-L218

Vulnerability details

[2022-09-Y2K] Users's funds can be drained because of wrong condition in function withdraw()

tags: c4, 2022-09-y2k, medium

Affected code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L215-L218

Impact

In function withdraw() of contract Vault, this condition (in affected code) will check the approval of receiver instead of msg.sender. So attacker can call withdraw with receiver is any contract that owner approve to (like contract Vault), although attacker has no approval from owner. It will make users loss their funds that they deposited to contract Vault.

Proof of concept

• Alice is a user that approve all to contract Vault : isApprovedForAll(address(Alice), address(Vault)) = true
• Alice deposit to an epoch of contract Vault. Bob is attacker that have no approval from Alice, but he want to drain all Alice's fund.
• After epoch end, Bob call withdraw before Alice with receiver is address of contract Vault.
• Transaction will be successful because the condition in function withdraw() will check the approval of receiver instead of msg.sender. Then all Alice's WETH will be transfered to contract Vault and Alice will lose them forever.

Tools Used

Manual review

Recommended Mitigation Steps

Replace receiver to msg.sender in this condition:

if(
    msg.sender != owner &&
    isApprovedForAll(owner, msg.sender) == false)
    revert OwnerDidNotAuthorize(msg.sender, owner);

[2022-09-Y2K] gas reports

tags: c4, 2022-09-y2k, gas

should replace i++ to ++i

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L443

no need to intialize variable = 0

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L159

redundant call

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L223