Signature does not have expiration

[hats-bug-reporter[bot]]

Github username: @https://github.com/sekkiat Twitter username: -- Submission hash (on-chain): 0xd3064ee5c12251e0d59ddb10b495019e1a48387128e208c62db9c67fec83856f Severity: medium

Description: Description\ The withdraw function in Bfx.sol does not implement expiration protection for the signed transaction, which allow the user to execute the transaction after a long time.

Attachments

1. Proof of Concept (PoC) File

 function testSignatureExpiration() public {
    SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({
        id: 1,
        trader: _claimant,
        amount: 1e18
    });

    //hash
    bytes32 digest = _sigUtils.getTypedDataHash(withdrawal);      
    //sign with the key
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(_settlerPrivateKey, digest);
    skip(3600*24*30); //one month
    assertGe(block.timestamp, 3600*24*30);
    _bfx.withdraw(
        withdrawal.id,
        withdrawal.trader,
        withdrawal.amount,
        v,
        r,
        s
    );
    //success
    assertEq(_token.balanceOf(withdrawal.trader), withdrawal.amount);
}

Remediation

• Add expiration param to the signature.

[alex-sumner]

This is not a bug. A trader with a valid signature is not required to perform the withdrawal immediately. It causes no problem if they wait a long time before making the withdrawal.

[sekkiat]

Hi Alex,

After some further research, I believe that without implementing expiration, it might allow the withdrawer to withdraw an unexpected token amount.

For example, initially, the payment token is Dummy Token. After the signer signs the transaction without executing it, if the owner changes the payment token to another token during this time and we run the signed transaction again, we will receive the other token instead of Dummy Token, causing some kind of imbalance?

Here is the updated POC:

1. Create a OtherToken.sol (Copied from the DummyToken)

   
   // SPDX-License-Identifier: MIT
   pragma solidity ^0.8.0;

import "../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";

contract OtherToken is ERC20 { address public immutable owner;

constructor() ERC20("OtherToken", "OTHER") {
    owner = msg.sender;
}

function mint(address account, uint256 amount) external virtual {
    require(msg.sender == owner, "ONLY_OWNER");
    _mint(account, amount);
}

function decimals() public view virtual override returns (uint8) {
    return 6;
}

}

2. Run the test below. The withdrawer receive OtherToken instead of the initial payment token.

   function testSignatureExpiration() public {
    SigUtils.Withdrawal memory withdrawal = SigUtils.Withdrawal({
        id: 1,
        trader: _claimant,
        amount: 1e18
    });
    //before
    assertEq(_token.balanceOf(withdrawal.trader), 0);
    assertEq(_othertoken.balanceOf(withdrawal.trader), 0);
    //hash
    bytes32 digest = _sigUtils.getTypedDataHash(withdrawal);      
    //sign with the key
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(_settlerPrivateKey, digest);
    //payment token changed
    vm.prank(_owner);
    _bfx.setPaymentToken(address(_othertoken));
    skip(3600*24*30); //after one month
    assertGe(block.timestamp, 3600*24*30);
    _bfx.withdraw(
        withdrawal.id,
        withdrawal.trader,
        withdrawal.amount,
        v,
        r,
        s
    );
    //after
    assertEq(_token.balanceOf(withdrawal.trader),0);
    assertEq(_othertoken.balanceOf(withdrawal.trader), withdrawal.amount);
}

[alex-sumner]

The intended use is that the payment token is set on deployment and never changes. On Blast it will be USDB. The ability to change it is restricted to the owner and this would only be done under unexpected circumstances, such as Blast deploying a new USDB token and deprecating the old one. It could only be used to switch to another dollar stable coin with the same number of decimals. Having an expiration on the signature would not help in any case. If the signature expired then there would have to be some mechanism to allow the user to make a new withdrawal, otherwise their funds would be lost, and the same issue would arise with the new withdrawal.