sherlock-audit / 2023-06-symmetrical-judging

5 stars 4 forks source link

AkshaySrivastav - Dust amount due to 18 decimals precision can be combined to pull out whole token amounts. #319

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

AkshaySrivastav

high

Dust amount due to 18 decimals precision can be combined to pull out whole token amounts.

Summary

Dust amount due to 18 decimals precision and token decimals precision conversion can be combined together to pull out whole token amounts.

Vulnerability Detail

AccountFacetImpl.sol

    function withdraw(address user, uint256 amount) internal {
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();
        GlobalAppStorage.Layout storage appLayout = GlobalAppStorage.layout();
        require(
            block.timestamp >=
            accountLayout.withdrawCooldown[msg.sender] + MAStorage.layout().deallocateCooldown,
            "AccountFacet: Cooldown hasn't reached"
        );
        uint256 amountWith18Decimals = (amount * 1e18) /
        (10 ** IERC20Metadata(appLayout.collateral).decimals());
        accountLayout.balances[msg.sender] -= amountWith18Decimals;
        IERC20(appLayout.collateral).safeTransfer(user, amount);
    }

The protocol operates on 18 decimal precision values. So when 100 USDC (with 6 decimals) are deposited, the balances and allocated balances of user are represented as 100e18.

Consider this scenario:

  1. PartyA and partyB both deposit 100 USDC each. These values are represented as 100e18 in contract.
  2. PartyA realizes a profit of 10.777777777777777777. Total funds of partyA are 110.777777777777777777 and partyB are 89.222222222222222223.
  3. Both parties close the position and withdraw funds completely.
  4. During withdrawal these 18 decimal precision amounts are converted into USDC amounts. Due to which partyA will still have 0.000000777777777777 as his balance in the symmetrical protocol.
  5. Now, partyA again repeat steps 1, 2 & 3, again gaining an additional 0.000000777777777777 value as his balance.
  6. Total additional balance of partyA now becomes 0.000000777777777777 + 0.000000777777777777 = 0.000001555555555554, which can be claimed for 0.000001 USDC.
  7. Even after claiming the additional free USDC, partyA will still have 0.000000555555555554 as his protocol balance. The same cycle can be repeated again.

The issue here is that the protocol leaves dust balance into user balance accounting, these dust values can be combined together to claim free money from the protocol. Moreover, the free money will come from the USDCs deposited by other users into the protocol.

While this accounting issue may seem minute, an incentivized attack to collect dust amounts across all users accrued over a long period of time can cause serious dents in protocol accounting. This can cause bank run on the protocol in which the users who claim at last suffer major losses.

Impact

Explained above

Code Snippet

https://github.com/sherlock-audit/2023-06-symmetrical/blob/main/symmio-core/contracts/facets/Account/AccountFacetImpl.sol#L27-L39

Tool used

Manual Review

Recommendation

Consider resetting user balances to 0 when their balance goes below a particular threshold. Also it should always be validated that a protocol must never push out more funds than amount of funds coming in (like always round in favor of protocol).

akshaysrivastav commented 1 year ago

Escalate for 10 USDC

I do believe this is a valid issue. As explained in the report, the protocol distributes free money to users, i.e., the protocol distributes more funds than what originally came in, which is a major accounting issue. The report also explains the scenario & steps for the issue to occur.

I would like to request the judge and sponsors to review this report again. The severity can debated between high & medium but the issue should be valid. Thanks.

sherlock-admin2 commented 1 year ago

Escalate for 10 USDC

I do believe this is a valid issue. As explained in the report, the protocol distributes free money to users, i.e., the protocol distributes more funds than what originally came in, which is a major accounting issue. The report also explains the scenario & steps for the issue to occur.

I would like to request the judge and sponsors to review this report again. The severity can debated between high & medium but the issue should be valid. Thanks.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

MoonKnightDev commented 1 year ago

This amount of 0.000000777777777777 is indeed the user's balance, and it is within their rights to withdraw it. However, due to decimal limitations in USDC, they cannot withdraw it. It's not another user's fund, nor is it free money.

hrishibhat commented 1 year ago

Result: Low Unique Considering this issue a valid low as the impact is not significant enough based on the above comment.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: