sherlock-audit / 2023-04-hubble-exchange-judging

7 stars 6 forks source link

Shubham - Buyer can buy all available collateral from an ongoing auction for almost 99% discount leading to massive loss of funds #205

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Shubham

high

Buyer can buy all available collateral from an ongoing auction for almost 99% discount leading to massive loss of funds

High

Summary

A buyer can buy collateral from an ongoing auction at the current auction price using buyCollateralFromAuction in the contract InsuranceFund.sol. But it turns out that as the time of the auction increases, the startPrice keeps on decreasing until a time comes when the price is almost negligible when the auction is about to end. The user can wait until the expiry time of the auction & call the function buyCollateralFromAuction & pay almost negligible amount to acquire all the tokens in the auction.

Vulnerability Detail

The buyCollateralFromAuction function calls _calcVusdAmountForAuction to calculate the vusd amount to transfer.

function buyCollateralFromAuction(address token, uint amount) override external {
        Auction memory auction = auctions[token];
        // validate auction
        require(_isAuctionOngoing(auction.startedAt, auction.expiryTime), "IF.no_ongoing_auction");

        // transfer funds
        uint vusdToTransfer = _calcVusdAmountForAuction(auction, token, amount);   ------> @audit
        address buyer = _msgSender();
        vusd.safeTransferFrom(buyer, address(this), vusdToTransfer);
        IERC20(token).safeTransfer(buyer, amount); // will revert if there wasn't enough amount as requested
        ................
        }
    }

_calcVusdAmountForAuction calls _getAuctionPrice to get the current price at the auction.

function _calcVusdAmountForAuction(Auction memory auction, address token, uint amount) internal view returns(uint) {
        uint price = _getAuctionPrice(auction);
        uint _decimals = ERC20Detailed(token).decimals();  // will fail if .decimals() is not defined on the contract
        return amount * price / 10 ** _decimals;
    }

The issue lies here.

function _getAuctionPrice(Auction memory auction) internal view returns (uint) {
        uint diff = auction.startPrice * (_blockTimestamp() - auction.startedAt) / auctionDuration;
        return auction.startPrice - diff;

The auctionDuration is fixed at 2 hours. (7200 sec) Lets take the following scenario into consideration:

Code Snippet

https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/InsuranceFund.sol#L184-L199 https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/InsuranceFund.sol#L297-L301 https://github.com/sherlock-audit/2023-04-hubble-exchange/blob/main/hubble-protocol/contracts/InsuranceFund.sol#L286-L289

Impact

Using this vulnerability, the buyer can never be liquidated as it has excess collateral to save itself from any loss that occurs & this vulnerability leads to loss of funds to the protocol.

Tool used

Manual Review

Recommendation

Calculate the auction price such that it can't go below a certain point or percentage.

ctf-sec commented 1 year ago

Not a duplicate because the report does not mention the transfer dust amount exploit path

ctf-sec commented 1 year ago

Comment from senior watson:

This isn't a dupe of 168. Invalid as it just describes how a dutch auction works. Since markets are assumed to be efficient it would never get to 99% discount unless that is the real value of the asset

Shubh0412 commented 1 year ago

Escalate The mentioned finding doesn't need an attacker to plan out a attack.

The protocol mainly uses Vusd/husd. The Auction is for "Collaterals" only. Suppose someone has approved a ERC20 collateral that isn't so popular among traders but only a few. The user gets liquidated & the collateral is up for grabs from the auction. Because the collateral isn't frequently traded as much, most of the participants will ignore the auction. But a guy with the same ERC20 collateral would just have to wait for the time to pass to buy it at a cheaper price. He can even buy it in small amounts during the duration of the auction incase someone sweeps in & takes the whole chunk. Even in that case, that user also gets it at a very lower price. Plus not to forget that there isn't only one auction going on at a time. There can be several auctions taking place at once & the possibility of this scenario being played out is very likely.

sherlock-admin2 commented 1 year ago

Escalate The mentioned finding doesn't need an attacker to plan out a attack.

The protocol mainly uses Vusd/husd. The Auction is for "Collaterals" only. Suppose someone has approved a ERC20 collateral that isn't so popular among traders but only a few. The user gets liquidated & the collateral is up for grabs from the auction. Because the collateral isn't frequently traded as much, most of the participants will ignore the auction. But a guy with the same ERC20 collateral would just have to wait for the time to pass to buy it at a cheaper price. He can even buy it in small amounts during the duration of the auction incase someone sweeps in & takes the whole chunk. Even in that case, that user also gets it at a very lower price. Plus not to forget that there isn't only one auction going on at a time. There can be several auctions taking place at once & the possibility of this scenario being played out is very likely.

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.

JeffCX commented 1 year ago

Comment from senior watson:

This isn't a dupe of 168. Invalid as it just describes how a dutch auction works. Since markets are assumed to be efficient it would never get to 99% discount unless that is the real value of the asset

Agree with senior watson

But a guy with the same ERC20 collateral would just have to wait for the time to pass to buy it at a cheaper price.

Other buyer may interested in buying the asset at current price level

Shubh0412 commented 1 year ago

Other buyer may interested in buying the asset at current price level

But the interested buyer is seeing that with every passing minute, the price is decreasing gradually, so why not wait a little longer to get the maximum discount & buy at a much cheaper price. The buyer doesn't have to plan an exploit to gain a favour just for himself. The given issue is applicable to any auction that occurs. I do understand how a Dutch auction works but in the case where no one decides to buy the offered collateral, its price plummets to zero which would lead to a heavy loss to the treasury. Not everyone is ready to buy whatever is auctioned.

hrishibhat commented 1 year ago

@Shubh0412 clearly don't see any vulnerability, seems like you are pointing out the drawbacks of a system. Not sure if I'm missing something here. @ctf-sec

ctf-sec commented 1 year ago

@Shubh0412 clearly don't see any vulnerability, seems like you are pointing out the drawbacks of a system. Not sure if I'm missing something here. @ctf-sec

Agree

hrishibhat commented 1 year ago

Result: Invalid Unique Considering this a non issue based on the comments above

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: