Aycozzynfada - Allocate() is broken due to incorrect precision

[sherlock-admin4]

Aycozzynfada

High

Allocate() is broken due to incorrect precision

Summary

AccountFacet.Allocate() doesn't scale the inputted amount correctly, therefore failing to allocate enough funds, this is because allocated amounts are stored in 18 decimal units whereas the allocate() fails to adjust the amount allocated to 18 decimals before adding to state.

In a similar function; depositAndAllocate(); the appropriate checks were made and the allocate() was called with 18 decimals after deposit.

Root Cause

-Root cause is in AccountFacet.allocate() which doesn't adjust the allocated amount to 18 decimals before calling AccountFacetImpl.allocate -the requisite adjustments were also not made in the AccountFacetImpl.allocate

//AccountFacetImpl.allocate
function allocate(uint256 amount) internal {
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();
        require(
            accountLayout.allocatedBalances[msg.sender] + amount <= GlobalAppStorage.layout().balanceLimitPerUser,
            "AccountFacet: Allocated balance limit reached"
        );
        require(accountLayout.balances[msg.sender] >= amount, "AccountFacet: Insufficient balance");
        accountLayout.balances[msg.sender] -= amount;
        accountLayout.allocatedBalances[msg.sender] += amount;
    }

Impact

Any users utilizing AccountFacet.allocate() to allocate 1000 USDC will end up only having 0.000000001 USDC allocated to their account, which is a very small fraction of their deposit. This might potentially lead to unexpected loss of funds due to the broken functionality if they rely on the accuracy of the outcome to perform certain actions dealing with funds/assets.

This case is similar to issues from from previous audit contest, the user expects to have full amount deposited and allocated, but ends up with only dust amount allocated, which can lead to unexpected liquidations (for example, user is at the edge of liquidation, calls Allocate to improve account health, but is liquidated instead). For consistency reasons, since this is almost identical to 222, should also be high.

PoC

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Account/AccountFacet.sol#L45-L64 https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L38-L47 Below is the AcountFacet.allocate without the requisite adjustments

//AcountFacet.allocate
function allocate(uint256 amount) external whenNotAccountingPaused notSuspended(msg.sender) notLiquidatedPartyA(msg.sender) {
        AccountFacetImpl.allocate(amount);//Valid medium
        emit AllocatePartyA(msg.sender, amount, AccountStorage.layout().allocatedBalances[msg.sender]);
        emit AllocatePartyA(msg.sender, amount); // For backward compatibility, will be removed in future
        emit SharedEvents.BalanceChangePartyA(msg.sender, amount, SharedEvents.BalanceChangeType.ALLOCATE);
    }

whereas with depositAndAllocate(), the amount was adjusted before calling AccountFacetImpl.allocate

//AcountFacet.depositAndAllocate

function depositAndAllocate(uint256 amount) external whenNotAccountingPaused notLiquidatedPartyA(msg.sender) notSuspended(msg.sender) {
        AccountFacetImpl.deposit(msg.sender, amount);
        uint256 amountWith18Decimals = (amount * 1e18) / (10 ** IERC20Metadata(GlobalAppStorage.layout().collateral).decimals());
        AccountFacetImpl.allocate(amountWith18Decimals);
        emit Deposit(msg.sender, msg.sender, amount);
        emit AllocatePartyA(msg.sender, amountWith18Decimals, AccountStorage.layout().allocatedBalances[msg.sender]);
        emit AllocatePartyA(msg.sender, amountWith18Decimals); // For backward compatibility, will be removed in future
        emit SharedEvents.BalanceChangePartyA(msg.sender, amountWith18Decimals, SharedEvents.BalanceChangeType.ALLOCATE);
    }

Mitigation

Scale the amount to internal accounting precision (18 decimals) before passing it AccountFacetImpl.allocate

function allocate(uint256 amount) external whenNotAccountingPaused notSuspended(msg.sender) notLiquidatedPartyA(msg.sender) {
    ++  uint256 amountWith18Decimals = (amount * 1e18) / (10 ** IERC20Metadata(GlobalAppStorage.layout().collateral).decimals());
        ++  AccountFacetImpl.allocate(amountWith18Decimals);
        emit AllocatePartyA(msg.sender, amount, AccountStorage.layout().allocatedBalances[msg.sender]);
        emit AllocatePartyA(msg.sender, amount); // For backward compatibility, will be removed in future
        emit SharedEvents.BalanceChangePartyA(msg.sender, amount, SharedEvents.BalanceChangeType.ALLOCATE);
    }