hash - Downcasting to uint96 can cause assets to be lost for some tokens

[sherlock-admin4]

hash

medium

Downcasting to uint96 can cause assets to be lost for some tokens

Summary

Downcasting to uint96 can cause assets to be lost for some tokens

Vulnerability Detail

After summing the individual bid amounts, the total bid amount is downcasted to uint96 without any checks

            settlement_.totalIn = uint96(result.totalAmountIn);

uint96 can be overflowed for multiple well traded tokens:

Eg:

shiba inu : current price = $0.00003058 value of type(uint96).max tokens ~= 2^96 * 0.00003058 / 10^18 == 2.5 million $

Hence auctions that receive more than type(uint96).max amount of tokens will be downcasted leading to extreme loss for the auctioner

Impact

The auctioner will suffer extreme loss in situations where the auctions bring in >uint96 amount of tokens

Code Snippet

downcasting totalAmountIn to uint96 https://github.com/sherlock-audit/2024-03-axis-finance/blob/cadf331f12b485bac184111cdc9ba1344d9fbf01/moonraker/src/modules/auctions/EMPAM.sol#L825

Tool used

Manual Review

Recommendation

Use a higher type or warn the user's of the limitations on the auction sizes

[0xJem]

Duplicate of #34

[Oighty]

Pretty similar to #209. Might be a duplicate.

[nevillehuang]

Agree both hinges on a high totalAmountIn

[kosedogus]

Escalate

Since there are minutes until the end of auction period, I might miss something, if that is the case sorry about that.

_settle calls _getLotMarginalPrice to get the totalAmountIn. The loop which adds amountIn's to totalAmountIn does not add every individual bid, if the latest bid filled the capacity loop breaks. capacity is taken from lotData as we can see:

uint256 capacity = lotData[lotId_].capacity;

And the capacity in lotData is uint96:

    mapping(uint96 id => Lot lot) public lotData;
...
    struct Lot {
        uint48 start; // 6 +
        uint48 conclusion; //
        uint8 quoteTokenDecimals;
        uint8 baseTokenDecimals;
        bool capacityInQuote;
        uint96 capacity;
        uint96 sold;
        uint96 purchased;
        uint96 partialPayout;
    }

Hence the capacity itself is below max value of uint96 inherently, and if we exceed capacity with the latest bid, then loop breaks. So what happens to latest bid? It's bidId is recorded and it is only partially filled, the excess is removed from totalAmountIn as we can see below:

                if (result.capacityExpended >= capacity) {
                    result.marginalPrice = price;
                    result.marginalBidId = bidId;
                    if (result.capacityExpended > capacity) {
                        result.partialFillBidId = bidId;
                    }
                    break;

            if (result.partialFillBidId != 0) {
                // Load routing and bid data
                Bid storage bidData = bids[lotId_][result.partialFillBidId];

                // Set the bidder on for the partially filled bid
                settlement_.pfBidder = bidData.bidder;
                settlement_.pfReferrer = bidData.referrer;

                // Calculate the payout and refund amounts
                uint256 fullFill =
                    Math.mulDivDown(uint256(bidData.amount), baseScale, result.marginalPrice);
                uint256 excess = result.capacityExpended - capacity;
                settlement_.pfPayout = uint96(fullFill - excess);
                settlement_.pfRefund =
                    uint96(Math.mulDivDown(uint256(bidData.amount), excess, fullFill));

                // Reduce the total amount in by the refund amount
                result.totalAmountIn -= settlement_.pfRefund;

Hence it seems like totalAmountIn can not possibly pass capacity which is uint96. If it can not pass uint96, there can't be any overflow.

[sherlock-admin2]

Escalate

Since there are minutes until the end of auction period, I might miss something, if that is the case sorry about that.

_settle calls _getLotMarginalPrice to get the totalAmountIn. The loop which adds amountIn's to totalAmountIn does not add every individual bid, if the latest bid filled the capacity loop breaks. capacity is taken from lotData as we can see:

uint256 capacity = lotData[lotId_].capacity;

And the capacity in lotData is uint96:

    mapping(uint96 id => Lot lot) public lotData;
...
    struct Lot {
        uint48 start; // 6 +
        uint48 conclusion; //
        uint8 quoteTokenDecimals;
        uint8 baseTokenDecimals;
        bool capacityInQuote;
        uint96 capacity;
        uint96 sold;
        uint96 purchased;
        uint96 partialPayout;
    }

Hence the capacity itself is below max value of uint96 inherently, and if we exceed capacity with the latest bid, then loop breaks. So what happens to latest bid? It's bidId is recorded and it is only partially filled, the excess is removed from totalAmountIn as we can see below:

                if (result.capacityExpended >= capacity) {
                    result.marginalPrice = price;
                    result.marginalBidId = bidId;
                    if (result.capacityExpended > capacity) {
                        result.partialFillBidId = bidId;
                    }
                    break;

            if (result.partialFillBidId != 0) {
                // Load routing and bid data
                Bid storage bidData = bids[lotId_][result.partialFillBidId];

                // Set the bidder on for the partially filled bid
                settlement_.pfBidder = bidData.bidder;
                settlement_.pfReferrer = bidData.referrer;

                // Calculate the payout and refund amounts
                uint256 fullFill =
                    Math.mulDivDown(uint256(bidData.amount), baseScale, result.marginalPrice);
                uint256 excess = result.capacityExpended - capacity;
                settlement_.pfPayout = uint96(fullFill - excess);
                settlement_.pfRefund =
                    uint96(Math.mulDivDown(uint256(bidData.amount), excess, fullFill));

                // Reduce the total amount in by the refund amount
                result.totalAmountIn -= settlement_.pfRefund;

Hence it seems like totalAmountIn can not possibly pass capacity which is uint96. If it can not pass uint96, there can't be any overflow.

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.

[kosedogus]

Since there are minutes until the end of auction period, I might miss something, if that is the case sorry about that.

:smile:

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Axis-Fi/moonraker/pull/130

[nevillehuang]

@kosedogus I do not quite get your escalation point. Maybe a PoC could help me decipher it. I see a clear loss of funds here from downcasting.

Cc @10xhash @Oighty

[Oighty]

Adding a bunch of uint96 amounts together can exceed type(uint96).max so casting totalAmountIn from a uint256 to a uint96 can overflow.

[kosedogus]

What I was saying, capacity is itself uint96.During loop that adds amountIn's together, everything copied as a uint256 and calculations are done with uint256 so that overflow won't occur. If adding a bid to totalAmountIn made it pass capacity (which is normally uint96, but for the purpose of preventing overflow it is copied as uint256 before this check), then loop breaks. The amount that exceeds capacity removed from totalAmountIn before it is downcasted to uint96. So totalAmountIn can be at most same with capacity in the end, which is uint96. So there won't be overflow.

[Evert0x]

@nevillehuang any reply to the latest comment?

[nevillehuang]

@kosedogus @10xhash Could you guys verify the escalation comment? Based on comment here overflow is still possible no on the last bid added correct? I think a PoC could verify the claim and the issue.

[10xhash]

@kosedogus @10xhash Could you guys verify the escalation comment? Based on comment here overflow is still possible no on the last bid added correct? I think a PoC could verify the claim and the issue.

The capacity is the amount of base token the seller wants to sell while amountIn is the amount of quote tokens that are paid by buyers. So the uint96 constrain on capacity is not related with totalAmountIn

Eg: uint96 capacity = 3 1e6 1e18; // 3million uint price = 32702 ; // price of usd in shiba uint totalAmountIn = capacity * price == 98106000000000000000000000000; // > uint96.max

this totalAmountIn can be sum of smaller bids ie. 10 bids each of 9810600000000000000000000000, where each is less than uint96.max

[kosedogus]

Yeah it was an oversight from my side I guess, thank you for clarification :)

[0xJem]

totalAmountIn is the sum of amountIn from bids (each of which is maximum uint96), and can overflow uint96. The lot capacity is unrelated to this.

[10xhash]

The protocol team fixed this issue in the following PRs/commits: Axis-Fi/moonraker#130

Fixed uint256 is now used avoiding the unsafe casting

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.

[Evert0x]

@kosedogus do I understand correctly that you agree the issue is valid?

[kosedogus]

@Evert0x yes sir

[Evert0x]

Result: High Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• kosedogus: rejected