code-423n4 / 2022-12-prepo-findings

0 stars 1 forks source link

Collateral tokens with small amount of decimals will not work #333

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L47-L48 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L66-L67

Vulnerability details

Tokens with a small amount of decimals might result in a revert when calculating _fee in the deposit() function of Collateral.sol. The fee is calculated as follows:

uint256 _fee = (_amount * depositFee) / FEE_DENOMINATOR;

Let’s assume the base token is EURS wich has 2 decimals (https://etherscan.io/token/0xdb25f211ab05b1c97d595516f45794528a807ad8#readProxyContract#F4).

When depositing 9 EURS while the collateral deposit fee is 0,1%, the fee will be calculated as follows:

uint256 _fee = 900 * 1000 / 1000000 = 0

This will result in a _fee of 0. Subsequently, the next line will result in a revert, making it impossible for the user to deposit funds and receive collateral tokens.

The same applies to the withdraw function when withdrawing collateral tokens for the underlying base token.

Impact

Depending on the used base layer token, smaller amounts of the token can not be deposited for withdrawn for collateral. As most tokens are using 6 or more decimals, the impact is rated as medium.

Tool Used

Manual Review

Recommended Mitigation Steps

In order to support tokens with smaller decimals and still receive a fee, mulDivUp from FixedPointMathLib might be used (https://github.com/transmissions11/solmate/blob/main/src/utils/FixedPointMathLib.sol). This guarantees the fee is at least 1 in case the divide by FEE_DENOMINATOR might result in zero.

Picodes commented 1 year ago

Even in your scenario the minimal amount would be 10 EURS, so in the worst case users have to pay at least 0.01 EUR of fees.

Picodes commented 1 year ago

So I don't think this could reasonably break the contract's functionality

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

Picodes marked the issue as grade-b