Escher contest - JC's results

Lines of code

https://github.com/code-423n4/2022-12-escher/blob/main/src/minters/LPDA.sol#L105

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 migration wrapper. Inaccessible funds means loss of funds, which is Medium severity.

Proof of Concept

refund() uses payable.transfer()

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);
}

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

While the function uses msg.sender, the funds are tied to the address that deposited them (L104), and there is no mechanism to change the owner of the funds to an alternate address.

Tools Used

Manual review

Recommended Mitigation Steps

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