🔒 Refund Excess Token (If Any) To Sender in `BatchDistributor`

[pcaversaccio]

🔒 Refund Excess Token (If Any) To Sender in `BatchDistributor`

[pcaversaccio]

@jaglinux this is only one way to force ether into the contract. There are other ways that bypass the nonpayable __default__ feature. You can force ether into a contract using selfdestruct (since the destination account code - in this case the BatchDistributor code - is not executed), or you can set the address of the BatchDistributor as your coinbase address (i.e. the fee recipient of your staking rewards) - also in this case the ether is forced into the contract. The third possibility would be, that you deploy the BatchDistributor via CREATE2. In that case, you can pre-compute the address where BatchDistributor will be deployed, send there some ether, and deploy BatchDistributor thereafter. Tada, you got some excess ether locked. As you can see there are plenty of use cases that cannot be prevented at all, and if people force ether into this contract for any weird reason, the first calling distribute_ether will receive some excess ether. I don't think this justifies the additional costs of opcodes tracking total. Therefore, I will close this issue.

[jaglinux]

Thanks for the explanation ! appreciate this is captured in codebase.

[pcaversaccio]

There is no such case as excess tokens for distribute_token since this works differently as for msg.value. There is no native way to handle tokens in the EVM! First you have to approve the amount you want to transfer and thereafter the exact amount is transferred via transferFrom during the distribute_token function call. Only if all tokens are distributed, the call will succeed. You don't send tokens directly to the contract but it's a combination of approve and distribute_token.

[jaglinux]

Yeah, missed the "total" variable. It ensures that correct token amount is used in transferFrom and hence no excessive tokens are transferred.

[jaglinux]

To keep the implementation same across the 2 functions,
"Caller gets exact eth refund and not the whole contract balance."

diff --git a/src/utils/BatchDistributor.vy b/src/utils/BatchDistributor.vy
index 27fec25..13238d1 100644
--- a/src/utils/BatchDistributor.vy
+++ b/src/utils/BatchDistributor.vy
@@ -51,14 +51,16 @@ def distribute_ether(data: Batch):
            of tuples that contain each a recipient address &
            ether amount in wei.
     """
+    total: uint256 = empty(uint256)
     for txn in data.txns:
         # A low-level call is used to guarantee compatibility
         # with smart contract wallets. As a general pre-emptive
         # safety measure, a reentrancy guard is used.
         raw_call(txn.recipient, b"", value=txn.amount)
+        total += txn.amount
 
-    if (self.balance != empty(uint256)):
-        raw_call(msg.sender, b"", value=self.balance)
+    if (msg.value != total):
+        raw_call(msg.sender, b"", value=msg.value - total)

Contract can have more balance if ether is accidentally sent in constructor. Thats the only possible case.
Default function prevents unwanted donation anyways.
https://vyper.readthedocs.io/en/stable/control-structures.html?highlight=__default__#the-default-function
Just as in Solidity, Vyper generates a default function if one isn’t found, in the form of a REVERT call. Note that this still generates an exception and thus will not succeed in receiving funds.

This is an edge case (ether is accidentally sent in constructor), we can ignore it. Based on reply, I will close the issue.

[pcaversaccio]

In order to reflect this discussion in the codebase, I quickly added a comment via d2d8b9b.