Funds can be stolen because of approval + send

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

The fulfill transaction on the receiving chain first approves the txData.callTo contract with the toSend amount.

It then tries to call the addFunds and execute actions on txData.callTo.
When any of the calls reverts, the funds are sent to the txData.receivingAddress.

Note this is a different attack from "Funds are sent twice on callTo errors". Assume that issue was already fixed and it would only send out the transfer once.

The txData.callTo is user-controlled and an attacker can deploy a contract to this address and revert on all calls.
They then receive the funds once (or twice if other issue was not fixed) through the direct LibAsset.transferAsset(txData.receivingAssetId, payable(txData.receivingAddress), toSend) call.
Afterwards, as the approval was given to callTo, the attacker can send a transaction to their callTo contract to transferFrom(TransactionManager, toSend) again.

Impact

The receiver can get twice the amounts of funds that the router agreed to (and locked up).
This allows stealing all added liquidity in the contract by an attacker engaging in cross-chain transfers with themselves.

Recommended Mitigation Steps

Giving approvals to arbitrary contracts is very dangerous. It should probably be removed completely.
If it's really desired, at least reset the approval to 0 again at the end of the fulfill function such that callTo must have used transferFrom within the same transaction in the execute call.

[LayneHaber]

#31