sherlock-audit / 2024-06-symmetrical-update-2-judging

0 stars 0 forks source link

slowfi - Inconsistent Handling of Decimal Precision in Bridge Transaction Restoration #20

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

slowfi

High

Inconsistent Handling of Decimal Precision in Bridge Transaction Restoration

Summary

The restoreBridgeTransaction function in BridgeFacetImpl.sol fails to normalize amounts to the 18-decimal precision standard expected by AccountStorage during transaction restoration. This discrepancy arises because BridgeStorage stores amounts without decimal escalation, while AccountStorage assumes all balances are escalated. As a result, restoring a bridge transaction leads to incorrect balance updates in AccountStorage.

Vulnerability Detail

During the restoration process, the BridgeFacetImpl.sol#L90 directly adds a non-escalated amount (bridgeTransaction.amount - validAmount) to an escalated balance in AccountStorage. This action violates the expected 18-decimal precision standard used throughout the system.

Impact

The failure to normalize amounts during bridge transaction restoration results in inconsistent data in stored balances. This impacts the accuracy of account balances within AccountStorage.

Code Snippet

Non escalated amount storage on restoreBridgeTransaction

function restoreBridgeTransaction(uint256 transactionId, uint256 validAmount) internal {
    ...

    AccountStorage.layout().balances[bridgeLayout.invalidBridgedAmountsPool] += (bridgeTransaction.amount - validAmount);
    bridgeTransaction.status = BridgeTransactionStatus.RECEIVED;
    bridgeTransaction.amount = validAmount;
}

Reference for escalated amounts on account storage layout on transferToBridge

    BridgeTransaction memory bridgeTransaction = BridgeTransaction({
        id: currentId,
        amount: amount,
        user: user,
        bridge: bridge,
        timestamp: block.timestamp,
        status: BridgeTransactionStatus.RECEIVED
    });
    AccountStorage.layout().balances[user] -= amountWith18Decimals;
    bridgeLayout.bridgeTransactions[currentId] = bridgeTransaction;

Reference for escalated amounts on account storage layout on deposit

Reference for escalated amounts on account storage layout on withdraw

Tool used

Manual Review

Recommendation

Escalate (bridgeTransaction.amount - validAmount) before storing it in AccountStorage layout balances.

Duplicate of #5

sherlock-admin2 commented 3 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/SYMM-IO/protocol-core/pull/45

sherlock-admin2 commented 3 months ago

The Lead Senior Watson signed off on the fix.