hats-finance / Blast-Futures-Exchange-0x97895c329b950755566ddcdad3395caaea395074

0 stars 0 forks source link

# Bfx::deposit doesnt handle token with fee on transfer mechanism #57

Open hats-bug-reporter[bot] opened 4 months ago

hats-bug-reporter[bot] commented 4 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x6a58b42fa2e83c60b1b5d8e8003d089ea3e6a652355f1dbd3bd615403678f277 Severity: medium

Description:

Bfx::deposit doesnt handle token with fee on transfer mechanism

Description\ Bfx deposit doesnt handle tokens with fee on transfer mechanism.
Because of this the emitted events are wrong in the amount field, and this leads to bad accounting because the bfx token balance will be always lower than expected.

Attack Scenario\ If bfx uses a paymentToken with a fee on transfer mechanism then its balance will be always lower, leading to bad accounting.

Attachments

  1. Proof of Concept (PoC) File In test project's directory create contract with a 10% tx fee, set filename BurnToken.sol with the following content:
    
    // SPDX-License-Identifier: MIT
    import "../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
    import "forge-std/console.sol";
    pragma solidity ^0.8.2;

contract BurnToken is IERC20 { mapping(address => uint) public balances; mapping(address => mapping(address => uint)) public allowance;

uint public totalSupply = 100 * 10 ** 18;
string public name = "BURNTOKEN";
string public symbol = "BRN";
uint public decimals = 18;

address admin;
uint public taxFee=10;
uint feeCoins;

constructor() {
    balances[msg.sender] = totalSupply;
    admin = msg.sender;
}

function balanceOf(address account) public view returns(uint) {
    uint actualBalance = balances[account];
    return actualBalance;
}

function transfer(address to, uint amount) public returns(bool) {
    require(balanceOf(msg.sender) >= amount, "Not enough balance");

    feeCoins += (taxFee * amount) / 100;
    balances[to] += (amount * (100 - taxFee)) / 100;

    balances[msg.sender] -= amount;

    emit Transfer(msg.sender, to, amount);
    return true;
}

function transferFrom(address from, address to, uint value) public returns(bool) {
    require(balanceOf(from) >= value, "Balance is too low");
    require(allowance[from][msg.sender] >= value, "Allowance is too low");

    feeCoins += (taxFee * value) / 100;

    balances[to] += (value * (100 - taxFee)) / 100;
    balances[from] -= value;

    emit Transfer(from, to, value);
    return true;
}

function approve(address spender, uint value) public returns(bool) {
    allowance[msg.sender][spender] = value;
    emit Approval(msg.sender, spender, value);
    return true;
}

}

And in top of `BfxVault.t.sol` append the following dependencies  
```js
import "forge-std/console.sol";
import {BurnToken} from "../test/BurnToken.sol";
import "../lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";

And create the following test case, a user deposit amount to bfx;

    function testBfxDontHandleTokenTransferOnFee() public {
        // amount
        uint amount = 1234567;
        address user = address(0x123abc);

        // new token
        BurnToken _tokenOnFee;
        _tokenOnFee = new BurnToken();
        _tokenOnFee.transfer(user, amount*2);
        console.log("_tokenOnFee.balanceOf(user) ",_tokenOnFee.balanceOf(user));

        // change token
        vm.prank(_bfxOwner);
        _bfx.setPaymentToken(address(_tokenOnFee));

        // stake
        vm.startPrank(user);
        _tokenOnFee.approve(address(_bfx), amount);
        _bfx.deposit(amount);
        vm.stopPrank();

        // token balance is different than deposited an emited event
        console.log("_tokenOnFee.balanceOf(_bfx) ",_tokenOnFee.balanceOf(address(_bfx)));
        assertLt(_tokenOnFee.balanceOf(address(_bfx)),amount);
    }

Observe that balance of bfx is lower than expected

  1. Revised Code File (Optional) Implement a balance change in bfx and emit the changed balance in deposit function

    function deposit(uint256 amount) external nonReentrant {
        uint bal_before = paymentToken.balanceOf(address(this));
        bool success = makeTransferFrom(msg.sender, address(this) , amount);
        require(success, "TRANSFER_FAILED");
        uint bal_after = paymentToken.balanceOf(address(this));
    
        uint256 depositId = allocateDepositId();
        emit Deposit(depositId, msg.sender, bal_after-bal_before);
    }

Files:

alex-sumner commented 4 months ago

The contract is not intended for use with fee on transfer tokens. Adding checks for such tokens would only waste gas.