Popcorn contest - okkothejawa's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/VaultController.sol#L669

Vulnerability details

Impact

The access control check _verifyCreatorOwner falsely checks if the address is creator of the vault AND the owner of the vault controller, instead of checking if address is creator of the vault OR owner of the vault controller. As this access control function is utilized in many vital public functions of VaultController, and as the creator of a vault is not necessarily the owner of the Vault controller at all times, this issue may lead to denial of service problems.

Proof of Concept

if (msg.sender != metadata.creator || msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

As can be seen above, the check reverts if msg.sender is not the vault creator or if msg.sender is not the owner of the VaultController contract. The negation of this check, which would give the allowed state, is

msg.sender == metadata.creator && msg.sender == owner

which shows that check is actually checking if the caller is vault creator AND the owner of the vault controller.

Tools Used

Manual review

Recommended Mitigation Steps

Change the line to

    if (msg.sender != metadata.creator && msg.sender != owner) revert NotSubmitterNorOwner(msg.sender);

so it reverts if the caller is not the vault creator nor owner.