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

0 stars 0 forks source link

# BfxVault::stake doesnt handle token with fee on transfer mechanism #58

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

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

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

Description: Description BfxVault::stake 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 staked token balance will be always lower than expected.

Attack Scenario If BfxVault 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:
    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 stakes amount to bfxVault;

    function testBfxVaultDontHandleTokenFeeOnTransfer() public {
        uint amount = 10000;
        address user = address(0x123abc);

        BurnToken _tokenOnFee;
        _tokenOnFee = new BurnToken();
        _tokenOnFee.transfer(user, amount*2);

        // change token
        vm.prank(_vaultOwner);
        _vault.setPaymentToken(address(_tokenOnFee));

        vm.startPrank(user);
        _tokenOnFee.approve(address(_vault), amount*2);

        vm.expectEmit(true, false, false, true, address(_vault));
        emit Stake(1, user, amount);
        _vault.stake(amount);
        console.log("_vault balance ",_tokenOnFee.balanceOf(address(_vault.bfx())));

        assertEq(_token.balanceOf(user), 0);
        assertLt(_token.balanceOf(address(_bfx)), amount);
    }   

Observe the emited event is for amount amount however the balance is lower.

  1. Revised Code File (Optional)

Files:

alex-sumner commented 7 months ago

The contract is not intended to work with a fee on transfer token. Adding checks for such tokens would only waste gas.