code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/5d8be6aa0e8634fdb2f328b99076b0d05fefab73/src/minters/LPDA.sol#L99-L106

Vulnerability details

Impact

The use of payable.transfer() is heavily frowned upon because it can lead to the locking of funds. The transfer() call requires that the recipient has a payable callback, only provides 2300 gas for its operation. This means the following cases can cause the transfer to fail:

The contract does not have a payable callback
The contract's payable callback spends more than 2300 gas (which is only enough to emit something)
The contract is called through a proxy which itself uses up the 2300 gas

If a user falls into one of the above categories, they'll be unable to receive funds from the vault in a refund. Inaccessible funds means loss of funds, which is Medium severity.

Proof of Concept

refund() use payable.transfer(). While use msg.sender, the funds are tied to the address that deposited them (line 100), and there is no mechanism to change the owner of the funds to an alternate address.

    function refund() public {
        Receipt memory r = receipts[msg.sender];
        uint80 price = uint80(getPrice()) * r.amount;
        uint80 owed = r.balance - price;
        require(owed > 0, "NOTHING TO REFUND");
        receipts[msg.sender].balance = price;
        payable(msg.sender).transfer(owed);
    }

Tools Used

Manual inspection

Recommended Mitigation Steps

Use address.call{value:x}() instead

c4-judge commented 1 year ago

berndartmueller marked the issue as duplicate of #99