I wish to know if the function given below correctly transfers rewards from the contract caller to the required account or not. Is there anything to consider before calling this function from some source?
I'm assuming that I need to approve an amount that the contract can spend from. Can this be done once at the time of contract deployment? If so, how can I ensure that every address with the DISBURSER_ROLE gets approved, i.e. during role assignment?
Code to reproduce
function disburseReward(address to, uint256 rewardWei)
external
onlyRole(DISBURSER_ROLE)
returns (bool)
{
if (msg.sender.balance < gasleft()) revert("native balance < gas");
if (token.balanceOf(msg.sender) < rewardWei)
revert("token balance < reward");
if (!token.transferFrom(msg.sender, to, rewardWei))
revert("Token transfer failed.");
emit RewardClaimed(msg.sender, to, rewardWei);
return true;
}
I gave up trying to set up approvals for each DISBURSER_ROLE address and instead decided to refresh approvals at the time of disbursement. Catch is obviously the approval amount must be lower than the token's total supply, which is easy to set up.
I'm using this definition now:
/**
@dev Disburses `reward` to `claimant`.
Emits {Approval} events in case {disburserApproveAmount} changes.
Emits {RewardClaimed}.
*/
function disburseReward(address claimant, uint256 reward)
external
onlyRole(DISBURSER_ROLE)
whenNotPaused
returns (bool)
{
address disburser = msg.sender;
// checks
require(
disburser.balance >= gasleft(),
"RewardClaim: native bal < gas"
);
require(
token.balanceOf(disburser) >= reward,
"RewardClaim: token bal < reward"
);
// interactions
address tokenAddress = address(token);
uint256 currentAllowance = token.allowance(disburser, tokenAddress);
// Will always be true for a first-time disburser
if (currentAllowance < reward) {
if (
!token.increaseAllowance(
disburser,
disburserApproveAmount - currentAllowance
)
) {
revert("RewardClaim: approval failed.");
}
}
if (!token.transferFrom(disburser, claimant, reward))
revert("RewardClaim: claim failed.");
emit RewardClaimed(disburser, claimant, reward);
return true;
}
2)γAnd I think maybe increaseAllowance() is not correct either
3)γIs your reward token a standard ERC20 token? cause for some tokens, they do not return value for the function transferFrom(), maybe you can use safeTransferFrom
That second fragment had me in a pickle really because there's the "owner" and a "spender" and I think the owner is the disburser in this case? So I thought that maybe it's the token contract that's doing the spending. Should it be this contract instead?
I'm using ERC20Upgradeable (not the interface) so I have access to increaseAllowance. OZ's transferFrom does return a boolean, though it should revert on failure anyway (in which case that check is probably redundant). Will check that in some time.
I'm further planning to call this method from a backend hosted by a service called Moralis. Our backend code is closed source, but essentially it has a database of addresses which can receive rewards, so that's why it will call disburseReward.
I'll be handling that line of enquiries on their forum.
But it only has some code to deploy contracts rather than call functions, and for your contracts, I think it really needs some changes to work, for example: contracts/RewardClaim.sol
If you add the modifier initializer for the constructor(), I do not think you can call the following function initialize(), cause you have initialized your contract when deployed, so I think you should remove the modifier initializer after the constructor(), and then you can call initialize().
And there are also some errors need to fix, write more test cases to test.
That piece of code gets added by default when I use the OZ Wizard to make an ERC20Upgradeable token. I also think I'll want to initialise on construction for safety reasons mainly. I assume when upgrades are proxied their initialisers are treated as being called the first time?
So when I deploy on the local blockchain I see that the deployments are always deterministically choosing the same set of addresses. Is there a way to attach an already deployed contract in this manner, or do I have to deploy afresh each time?
What I meant was, once I have deployed a contract on Hardhat Network once using hardhat test or hardhat run deploy.ts, can I attach it? It seems like that won't be possible if the contracts are deployed on deterministic addresses.
Cause the disburser should approve this contract, and then this contract can transfer token from disburser to user.
And it seems like you use the hardhat, so I think you can use the console.sol in the contract to check.
In this case, I find that the transaction reverts:
BanzanToken
β Should have symbol ZAN (for now) (1090ms)
β Should not be able to attach to existing contract deployments for BanzanToken
RewardClaim
β Should not deploy RewardClaim if BanzanToken has no supply (170ms)
β Should deploy RewardClaim if BanzanToken has supply (1202ms)
1) Should disburse rewards
4 passing (4s)
1 failing
1) RewardClaim
Should disburse rewards:
AssertionError: Expected transaction NOT to be reverted
When run without the expect:
1) RewardClaim
Should disburse rewards:
Error: VM Exception while processing transaction: reverted with reason string 'ERC20: transfer amount exceeds allowance'
at BanzanToken.transferFrom (@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol:164)
at <UnrecognizedContract>.<unknown> (0xa513e6e4b8f2a923d98304ec87f64353c4d5c853)
at RewardClaim.disburseReward (contracts/RewardClaim.sol:148)
at <UnrecognizedContract>.<unknown> (0x8a791620dd6260079bf849dc5567adc3f2fdc318)
at HardhatNode._mineBlockWithPendingTxs (node_modules\hardhat\src\internal\hardhat-network\provider\node.ts:1650:23)
RewardClaim
β Should not deploy RewardClaim if BanzanToken has no supply (149ms)
β Should deploy RewardClaim if BanzanToken has supply (1211ms)
DEBUG: currentAllowance = 0, reward = 10000000000000
DEBUG: approval succeeded. Allowance = 0, disburserApproveAmount = 1e+22
1) Should disburse rewards
4 passing (4s)
1 failing
1) RewardClaim
Should disburse rewards:
Error: VM Exception while processing transaction: reverted with reason string 'ERC20: transfer amount exceeds allowance'
...
So I'm absolutely sure the allowance isn't going up.
Same error when I try token.increaseAllowance(selfAddress, disburserApproveAmount - currentAllowance), going by the allowance checking logic.
/**
* @dev See {IERC20-transferFrom}.
*
* Emits an {Approval} event indicating the updated allowance. This is not
* required by the EIP. See the note at the beginning of {ERC20}.
*
* Requirements:
*
* - `sender` and `recipient` cannot be the zero address.
* - `sender` must have a balance of at least `amount`.
* - the caller must have allowance for ``sender``'s tokens of at least
* `amount`.
*/
function transferFrom(
address sender,
address recipient,
uint256 amount
) public virtual override returns (bool) {
_transfer(sender, recipient, amount);
uint256 currentAllowance = _allowances[sender][_msgSender()];
require(currentAllowance >= amount, "ERC20: transfer amount exceeds allowance");
unchecked {
_approve(sender, _msgSender(), currentAllowance - amount);
}
return true;
}
/**
* @dev Atomically increases the allowance granted to `spender` by the caller.
*
* This is an alternative to {approve} that can be used as a mitigation for
* problems described in {IERC20-approve}.
*
* Emits an {Approval} event indicating the updated allowance.
*
* Requirements:
*
* - `spender` cannot be the zero address.
*/
function increaseAllowance(address spender, uint256 addedValue) public virtual returns (bool) {
_approve(_msgSender(), spender, _allowances[_msgSender()][spender] + addedValue);
return true;
}
Since both of these methods are called by RewardClaim, the msg.sender for both (since ContextUpgradeable defaults this value and _msgSender() isn't overriden in ERC20*) will remain the same. As such I think it's impossible to both approve an allowance and perform a transfer from the same contract.
Unless of course I don't know how msg.sender works, in which case, please educate me!
Code to reproduce
Environment