code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L230-L231 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L260-L261

Vulnerability details

Impact

File: Vault.sol

Functions: withdraw() and redeem()

When msg. sender is not the owner. Below line can cause an arithmetic underflow/overflow error when allowance is not sufficient. This should be gracefully handled.

 if (msg.sender != owner)
            _approve(owner, msg.sender, allowance(owner, msg.sender) - shares);

Proof of Concept

Here is a simple foundry test that I added to Vault.t.sol

function test__arithemetic_bug() public {
        asset.mint(alice, 1e18);
        vm.startPrank(alice);
        asset.approve(address(vault), 1e18);
        vault.deposit(1e18, alice);
        vm.stopPrank();

        assertEq(vault.balanceOf(alice), 1e18);
        assertEq(asset.balanceOf(alice), 0);
        vm.expectRevert(stdError.arithmeticError);
        vault.withdraw(1e18, bob, alice);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Add a check to make sure msg.sender has enough allowance from the owner. For example:


if( allowance(owner, msg.sender) < shares) revert InsufficientAllowance()

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)