Y2k Finance contest - peritoflores's results

Lines of code

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

Vulnerability details

Impact

Anyone can withdraw on behalf of approved user

Proof of Concept

Function withdraw at Vault.sol has incorrect access control. As the owner is passed as a parameter anyone can call withdraw to a approved receiver.

This is the same logic used in SemiFungibleVault.sol. Mathematically A or B == not A and not B

function withdraw(
uint256 id,
uint256 assets,
address receiver,
address owner
)
external
override
epochHasEnded(id)
marketExists(id)
returns (uint256 shares)
 {
if(
  msg.sender != owner &&
  isApprovedForAll(owner, receiver) == false)
  revert OwnerDidNotAuthorize(msg.sender, owner);

Recommended

The right logic is msg.sender is either the owner or msg.sender is approved .

require(
  msg.sender == owner || isApprovedForAll(owner, msg.sender),
  "Only owner or approved user can withdraw"
);

Then if you want the receiver not to be the msg.sender

require(receiver != address(0))

otherwise remove receiver from the input parameters