Skip to content
This repository has been archived by the owner on Mar 10, 2024. It is now read-only.

panprog - MultiAccount depositAndAllocateForAccount function doesn't scale the allocated amount correctly, failing to allocate enough funds #15

Open
sherlock-admin opened this issue Sep 4, 2023 · 7 comments
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Sep 4, 2023

panprog

high

MultiAccount depositAndAllocateForAccount function doesn't scale the allocated amount correctly, failing to allocate enough funds

Summary

This is an issue very similar to issue 222 from previous audit contest, but in a MultiAccount smart contract.

MultiAccount.depositAndAllocateForAccount uses the same amount value both for depositFor and for allocate. However, deposit amount decimals are from the collateral token while allocate amount decimals = 18. This means that for USDC (decimals = 6), depositAndAllocateForAccount will deposit correct amount, but allocate amount which is 1e12 times smaller (dust amount).

Vulnerability Detail

Internal accounting (allocatedBalances) are tracked as fixed numbers with 18 decimals, while collateral tokens can have different amount of decimals. This is correctly accounted for in AccountFacet.depositAndAllocate:

    AccountFacetImpl.deposit(msg.sender, amount);
    uint256 amountWith18Decimals = (amount * 1e18) /
        (10 ** IERC20Metadata(GlobalAppStorage.layout().collateral).decimals());
    AccountFacetImpl.allocate(amountWith18Decimals);

But it is treated incorrectly in MultiAccount.depositAndAllocateForAccount:

    ISymmio(symmioAddress).depositFor(account, amount);
    bytes memory _callData = abi.encodeWithSignature(
        "allocate(uint256)",
        amount
    );
    innerCall(account, _callData);

This leads to incorrect allocated amounts.

Impact

Similar to 222 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 depositAndAllocate to improve account health, but is liquidated instead). For consistency reasons, since this is almost identical to 222, it should also be high.

Code Snippet

The same amount is used for depositFor and allocate:
https://github.com/sherlock-audit/2023-08-symmetrical/blob/main/symmio-core/contracts/multiAccount/MultiAccount.sol#L167-L173

Tool used

Manual Review

Recommendation

Scale amount correctly before allocating it:

    ISymmio(symmioAddress).depositFor(account, amount);
+   uint256 amountWith18Decimals = (amount * 1e18) /
+       (10 ** IERC20Metadata(collateral).decimals());
    bytes memory _callData = abi.encodeWithSignature(
        "allocate(uint256)",
-       amount
+       amountWith18Decimals
    );
    innerCall(account, _callData);
@sherlock-admin
Copy link
Contributor Author

1 comment(s) were left on this issue during the judging contest.

0xyPhilic commented:

invalid because internally the depositAndAllocateForAccount calls the allocate function which does not use scaled amount but the actual input amount

@MoonKnightDev MoonKnightDev added Sponsor Confirmed The sponsor acknowledged this issue is valid Disagree With Severity The sponsor disputed the severity of this issue Will Fix The sponsor confirmed this issue will be fixed labels Sep 9, 2023
@MoonKnightDev
Copy link

No funds will be lost. The user simply needs to reallocate their balance. Therefore, the severity is not high since there's no loss of funds.

@nevillehuang
Copy link
Collaborator

Relating to this comment in the previous contest, it has the same root cause and potential same consequence, so could be valid H.

@MoonKnightDev
Copy link

Relating to this comment in the previous contest, it has the same root cause and potential same consequence, so could be valid H.

The root cause differs. Here, you can easily rectify the allocatedBalance by allocating again. Moreover, no funds are lost

@nevillehuang
Copy link
Collaborator

Relating to this comment in the previous contest, it has the same root cause and potential same consequence, so could be valid H.

The root cause differs. Here, you can easily rectify the allocatedBalance by allocating again. Moreover, no funds are lost

Ok can be valid M according to Impact mentioned in the submission.

@hrishibhat hrishibhat added Medium A valid Medium severity issue and removed High A valid High severity issue Disagree With Severity The sponsor disputed the severity of this issue labels Sep 18, 2023
@sherlock-admin sherlock-admin changed the title Calm Latte Hamster - MultiAccount depositAndAllocateForAccount function doesn't scale the allocated amount correctly, failing to allocate enough funds panprog - MultiAccount depositAndAllocateForAccount function doesn't scale the allocated amount correctly, failing to allocate enough funds Sep 18, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Sep 18, 2023
@MoonKnightDev
Copy link

Fixed Code PR: SYMM-IO/protocol-core#35

@xiaoming9090
Copy link
Collaborator

Verified. Fixed in PR 35.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants