vulnerability of this function using tx.origin

Question

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

Answer 1

One exploit I can think of is that you can trick another smart contracts into depositing on your behalf.

Let's say there is a Vault contract that pools ETH deposits and then deposits them into a revenue-generating contract like your Test contract. This contract could be exploited by having it call depositand then you withdraw the deposit to your personal wallet.

Answer 2

In the withdraw() function, the tx.origin is used to determine the address of the caller. However, this can be a security vulnerability because the tx.origin can be manipulated by a malicious contract to impersonate another address. This means that a malicious contract could potentially call the withdraw() function and transfer the funds belonging to another address to itself.

One way to improve the security of this contract is to use the msg.sender instead of tx.origin to determine the caller's address. The msg.sender is the actual address of the caller, and it cannot be manipulated by a malicious contract.

Additionally, as you mentioned, it would be good to explicitly set the visibility of the balance mapping to public so that it can be accessed by other contracts. This will allow other contracts to check the balance of an address without having to call the deposit() or withdraw() functions.

Finally, as you mentioned, it would be good to add a require() statement at the beginning of the withdraw() function to check that the caller has a non-zero balance. This will prevent the contract from executing the transfer if the caller has no funds to withdraw, which will save gas and prevent unnecessary transaction failures.

Here is an updated version of the contract that includes these improvements:

contract Test {
mapping(address => uint256) public balance;

  constructor() {
  }

function withdraw() external {
require(balance[msg.sender] > 0, "Cannot withdraw funds: no funds available");
(bool success, ) = msg.sender.call{value: balance[msg.sender]}("");
require(success, "transfer failed");
balance[msg.sender] = 0;
  }

  function deposit () external payable {
balance[msg.sender] = msg.value;
  }
}