Rounding error leads to loss of tokens when transferring tokens to the contract

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x6088f578250a0daaccace35a03351d199fd433516b65778ffafd593fe1654a51 Severity: high

Description: Description\ In the underwrite function, a rounding error occurs when transferring tokens to the contract due to integer division truncating decimals.

• The tokens amount to be sent to the contract is calculated by multiplying purchasedTokens by 1035 and dividing by 1000:

ERC20(toAsset).safeTransferFrom(
  msg.sender,
  address(this),
  purchasedTokens * (
    UNDERWRITING_COLLATERAL_DENOMINATOR + UNDERWRITING_COLLATERAL
  ) / UNDERWRITING_COLLATERAL_DENOMINATOR  
);

For example:

purchasedTokens is 112. UNDERWRITING_COLLATERAL_DENOMINATOR is 1000. UNDERWRITING_COLLATERAL is 35.

purchasedTokens * 1035 / 1000;

112 * 1.035 = 115.92 Solidity truncates, so 115 tokens is transferred instead of 116.

1. Proof of Concept (PoC) File

   https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L788-L795

2. Revised Code File (Optional)

   Amount received should be rounded up.

[reednaa]

1. It is going to be off by 1 wei at most. (or whatever you call the lowest value).
2. We only cover issues when the accuracy is bad AND the asset has more than 6 decimals. In case of 6 decmials and 1 Token, the difference is max 10^-6 = 0,0001% which is nothing.
3. The "soft fix" is simply to set UNDERWRITING_COLLATERAL higher. Unless you can provide very good arguments for why the constants are set incorrectly, it is a won't fix.

[ololade97]

It's not going to be off by 1 wei. It almost one token. purchasedTokens is stored in uint256.

For instance, in the example here: 112 * 1.035 = 115.92

.92 will be 1 token when 0.08 is added to it. This will result to a bad loss overtime as it will accumulate.

[ololade97]

This may require a "soft fix", but as it is, it's a vulnerability causing a loss. "won't fix" tag shouldn't be attached.

[reednaa]

Tokens have "decimals". What that means is when we are talking about 1 token, we are actually talking about 1 * 10**decimals units. That implies that 1 Ether = 1000000000000000000 (wei). So in your example:

Purchased tokens is 112000000000000000000 UNDERWRITING_COLLATERAL_DENOMINATOR is 1000. UNDERWRITING_COLLATERAL is 35.

112000000000000000000 * 1035 / 1000;

112000000000000000000 * 1.035 = 115920000000000000000

In your example there is no loss of precision.

[reednaa]

Feel free to post this in the Discord for a discussion or find another auditor to provide an independent opinion.

[ololade97]

No, the calculation is wrong.

purchasedTokens is stored in uint256. By this, if 112000000000000000000 is stored in purchasedTokens, it doesn't mean 112 anymore. It's a huge sum.

This is where the error in your calculation comes from.

[ololade97]

I've asked around. The conclusion was that ERC20 tokens won't have a rounding error here. However, tokens with small decimals may have a rounding issue.

[reednaa]

We limit issues to 6 decimals or more. (USDC equiv)