Daniel Von Fange
On June 9th, @lucash_dev found a vulnerability (writeup) in a Zapper contract, that would have allowed an attacker to steal all funds that the contract had been given permission to move on users' behalf.
Traditionally when you want a contract to be able to move your tokens, you must first send the ERC20 a transaction that approves the contract, and specifies how much it can move. This is a painful for two reasons - It costs another transactions worth of gas, and user has the delay of waiting for two transactions to succeed, rather than just one.
Because of this, “permit” functionality is becoming more and more popular in ERC20s. This allows the end user to sign some data locally giving permission for a contract to use their funds, then send this signed data to the contract that will be using the user's funds. This contract can then call “permit” on the ERC20 contract, passing along the users permission data and signature, which allows the contract to move the user's funds, all in a single transaction.
The affected Zapper contract was intended to allow the user to withdraw their LP tokens from a trusted set of pools. It was intended to be able to call permit on the pool before attempting to withdraw the user’s funds. Unfortunately, two functions in this contract would forward on to the pools any command which the user sent, as if the zapper contract itself had issued the user's command. This allowed an attacker to call methods other than permit on the pools, and to do so using the identity of the zapper contract.
It is as if a bank president allowed you to write any message you wanted on his letterhead, sign it with his own wax seal, and send it to whatever bank you wished. Instead of writing “Bob signed below and permits ABC Bank to withdraw $100 from his account”, you could instead write “Withdraw $10 million from ACME Corporation's account and transfer it to Bob.”
Here's one of the vulnerable functions. permitData is used as the message/command to send to the pool.
function ZapOut2PairTokenWithPermit(
address fromPoolAddress,
uint256 incomingLP,
address affiliate,
bytes calldata permitData
) external stopInEmergency returns (uint256 amountA, uint256 amountB) {
_validatePool(fromPoolAddress);
(bool success, ) = fromPoolAddress.call(permitData);
...snip...
}Don’t allow arbitrary user data to be sent to call() methods. Instead, if you need to communicate with an external contract, build the remote call yourself, to ensure that the right method is called, and the passed parameters are correct. For example:
// Don't do this!
function noNever(bytes calldata data) external {
lendingPool.call(data);
}
// Specify what method and what parameters are used on the other contract.
function yesThis(uint256 pid) external {
lendingPool.updateTokens(pid);
}The automated checker Slither will only mark the use of call as at an informational level, the lowest possible.
No. We only do arbitrary remote calls as a part of our governance / timelock contracts. Delegatecalls are only used as part of a proxy upgrade pattern.