Lack of support for fee-on-transfer token when handling the refunding payment

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L264
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L265

Vulnerability details

Impact

Lack of support for fee-on-transfer token when handling the refunding payment.

Proof of Concept

According to

https://biconomy.notion.site/Biconomy-SDK-adf0c6cedb08436097bf099b8f46aac7

Which ERC20 Tokens are supported as payments?

Biconomy relayers will initially support payments in stablecoins. In the future, Dapps can also participate in a relayer network and collect fees in tokens of their choice.

However, when handling the refund payment, the code logic does not support fee-on-transfer token.

When handling the refund payment inside the transaction execTransaction, handlePayment is called

if (refundInfo.gasPrice > 0) {
	//console.log("sent %s", startGas - gasleft());
	// extraGas = gasleft();
	payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
	emit WalletHandlePayment(txHash, payment);
}

which calls:

function handlePayment(
	uint256 gasUsed,
	uint256 baseGas,
	uint256 gasPrice,
	uint256 tokenGasPriceFactor,
	address gasToken,
	address payable refundReceiver
) private nonReentrant returns (uint256 payment) {
	// uint256 startGas = gasleft();
	// solhint-disable-next-line avoid-tx-origin
	address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
	if (gasToken == address(0)) {
		// For ETH we will only adjust the gas price to not be higher than the actual used gas price
		payment = (gasUsed + baseGas) * (gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
		(bool success,) = receiver.call{value: payment}("");
		require(success, "BSA011");
	} else {
		payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
		require(transferToken(gasToken, receiver, payment), "BSA012");
	}
	// uint256 requiredGas = startGas - gasleft();
	//console.log("hp %s", requiredGas);
}

note that when the gasToken is not address(0), we enter the code block:

payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
require(transferToken(gasToken, receiver, payment), "BSA012");

According to https://github.com/d-xo/weird-erc20#fee-on-transfer

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

then the amount of gasToken that receiver received is less than "payment" amount because a part of the amount is charged as transfer fee, then the receiver receive less amount than they entitled to.

Tools Used

Manual Review.

Recommended Mitigation Steps

We recommend the whitelist the gasToken to make sure the Dapps do not use fee-on-transfer token as the gas payment token.

[c4-judge]

Duplicate of #115

[c4-sponsor]

livingrockrises marked the issue as sponsor acknowledged

[c4-sponsor]

livingrockrises marked the issue as disagree with severity