I'm aware, and I easily understand why using tx.origin to check identity of a caller is vunerable to attack, as there is plethor of article about this, like this example
But here another case I couldn't find developed on article : is there a vulnerability in this situation ? I cannot see how we could steal someone else balance here. Even if this function is called from a malicious smart contract, at the end the EOA will get his balance. And I know this code means a smart contract cannot have a balance in this contract, meaning a multi-sig wallet will not be able to store value here too, but I wouldn't call this a vulnerability.
For the record this is from a quizz, and the question is "what is wrong/could be improved"
The 2 things I could find are to explicitly set visibility to the balance (to public
for example), and add a require(balance[tx.origin] > 0)
at the beginning to not consume gas if there's nothing to withdraw
contract Test {
mapping(address => uint256) balance;
constructor() {
}
function withdraw() external {
(bool success, ) = tx.origin.call{value: balance[tx.origin]}("");
require(success, "transfer failed");
balance[tx.origin] = 0;
}
function deposit () external payable {
balance[tx.origin] = msg.value;
}
}
Thank you