GenericHandler - Add additional depositor address argument
aalavandhan opened this issue · 2 comments
Downstream contracts that use GenericHandler
to transfer data to and from the bridge have no way of validating the depositor (or the initiator of the cross-bridge transaction).
To expose the depositor address to the downstream contract it would have to be passed through metadata
and this opens up the chance of a potential man in the middle attack.
# Generic handler
bytes4 sig = _contractAddressToDepositFunctionSignature[contractAddress];
if (sig != bytes4(0)) {
bytes memory callData = abi.encodePacked(sig, metadata);
(bool success,) = contractAddress.call(callData);
require(success, "delegatecall to contractAddress failed");
}
FIX
I propose the following change to add an additional parameter to the downstream contract invocation.
bytes memory callData = abi.encodePacked(sig, metadata, abi.encode(depositor));
Background
I've been working on a custom implementation to handle cross-bridge transfers of the Ampleforth token.
The Ampleforth behaves differently from other ERC-20 tokens, the balances of the wallet holders change automatically once a day through a Rebase operation. AMPLs are also non-dilutive, ie) the ratio of a wallet's balance to the total supply will be constant (provided tokens are not transfered in or out) regardless of how the balances change. To handle AMPL transfers across bridges safely, the amount is denominated by 2 numbers.
The amount of AMPL and the current total supply of AMPL.
We implement custom logic in gateway contracts on either side of the bridge which deal with minting, burning, locking, and unlocking tokens in rebase-safe denominations.
Since the downstream contract (ampl-gateway) triggered by GenericHandler has no ability to validate the depositor
address, it creates a risk of a man in the middle attack.
Thanks @nithinkrishna!
I think this makes a lot of sense. We intended for the call requirements to be minimal but it makes a lot of sense to have the function know who the caller is for validation. LGTM.
@nithinkrishna can you check another possible solution to this problem and leave feedback? #210