xiaoming90 - Inconsistent max payout returned from `getMarketInfoForPurchase` across different auctioneers

[sherlock-admin]

xiaoming90

medium

Inconsistent max payout returned from getMarketInfoForPurchase across different auctioneers

Summary

The max payout returned by getMarketInfoForPurchase function has a different meaning depending on the type of Auctioneer. Given these differences, any parties (internal or external) that rely on the getMarketInfoForPurchase function will have an issue because of the inconsistent result returned by the interface.

Vulnerability Detail

The BondBaseFPA.getMarketInfoForPurchase and BondBaseOFDA.getMarketInfoForPurchase functions retrieve the market's max payout directly from market.maxPayout.

File: BondBaseFPA.sol
314:     function getMarketInfoForPurchase(
315:         uint256 id_
316:     )
317:         external
318:         view
319:         returns (
320:             address owner,
321:             address callbackAddr,
322:             ERC20 payoutToken,
323:             ERC20 quoteToken,
324:             uint48 vesting,
325:             uint256 maxPayout
326:         )
327:     {
328:         BondMarket memory market = markets[id_];
329:         return (
330:             market.owner,
331:             market.callbackAddr,
332:             market.payoutToken,
333:             market.quoteToken,
334:             terms[id_].vesting,
335:             market.maxPayout
336:         );
337:     }

File: BondBaseOFDA.sol
422:     function getMarketInfoForPurchase(uint256 id_)
423:         external
424:         view
425:         returns (
426:             address owner,
427:             address callbackAddr,
428:             ERC20 payoutToken,
429:             ERC20 quoteToken,
430:             uint48 vesting,
431:             uint256 maxPayout
432:         )
433:     {
434:         BondMarket memory market = markets[id_];
435:         return (
436:             market.owner,
437:             market.callbackAddr,
438:             market.payoutToken,
439:             market.quoteToken,
440:             terms[id_].vesting,
441:             market.maxPayout
442:         );
443:     }

However, for the BondBaseOSDA.getMarketInfoForPurchase function, it was done differently. It calls the BondBaseOSDA.maxPayout function instead of accessing the market.maxPayout to obtain the max payout of a market.

File: BondBaseOSDA.sol
505:     function getMarketInfoForPurchase(uint256 id_)
506:         external
507:         view
508:         override
509:         returns (
510:             address owner,
511:             address callbackAddr,
512:             ERC20 payoutToken,
513:             ERC20 quoteToken,
514:             uint48 vesting,
515:             uint256 maxPayout_
516:         )
517:     {
518:         BondMarket memory market = markets[id_];
519:         return (
520:             market.owner,
521:             market.callbackAddr,
522:             market.payoutToken,
523:             market.quoteToken,
524:             terms[id_].vesting,
525:             maxPayout(id_)
526:         );
527:     }

The result returned from the BondBaseOSDA.maxPayout function is different from the market.maxPayout because if the capacity is lower than the max payout, the max payout will be capped at the remaining capacity. Refer to Line 573 below.

File: BondBaseOSDA.sol
561:     function maxPayout(uint256 id_) public view override returns (uint256) {
562:         // Get current price
563:         uint256 price = marketPrice(id_);
564: 
565:         BondMarket memory market = markets[id_];
566:         BondTerms memory term = terms[id_];
567: 
568:         // Convert capacity to payout token units for comparison with max payout
569:         uint256 capacity = market.capacityInQuote
570:             ? market.capacity.mulDiv(term.scale, price)
571:             : market.capacity;
572: 
573:         // Cap max payout at the remaining capacity
574:         return market.maxPayout > capacity ? capacity : market.maxPayout;
575:     }

Assume the max payout of an FPA, OFDA, and OSDA market is 100, and their current capacity is 50.

Calling BondBaseFPA.getMarketInfoForPurchase and BondBaseOFDA.getMarketInfoForPurchase functions will return 100 as the max payout, while calling BondBaseOSDA.getMarketInfoForPurchase function will return 50 as the max payout.

Impact

The max payout returned by getMarketInfoForPurchase function has a different meaning depending on the type of Auctioneer. For FPA and OFDA, it means the max payout that is configured during market initialization, and it stays constant. For OSDA, it means the maximum amount of payout tokens that can be issued to a user at this point in time, and it is not constant and takes into consideration of the current capacity of the market.

Given these differences, any parties (internal or external) that rely on the getMarketInfoForPurchase function will have an issue because of the inconsistent result returned by the interface.

Code Snippet

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/bases/BondBaseFPA.sol#L314

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/bases/BondBaseOFDA.sol#L422

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/bases/BondBaseOSDA.sol#L505

Tool used

Manual Review

Recommendation

Standardize the max payout returned from the getMarketInfoForPurchase function across all the auctioneers (FPA, OFDA, OSDA, SDA)

[Oighty]

Agree that this should be updated to be consistent across auctioneers. We will use the function value since it incorporates the capacity as a limit. The maxPayout value from this function is currently only used in a view function in the Aggregator.

[UsmannK]

Escalate for 10 USDC.

There is no impact of loss of funds shown in this issue. By the Sherlock criteria it does not qualify for a Medium.

Watson states as impact that "any parties (internal or external) that rely on the getMarketInfoForPurchase function will have an issue because of the inconsistent result returned by the interface." However this explanation is not concrete, does not include a POC, and does not show a scenario where loss of funds are possible.

Sponsor notes that the incorrect function is " only used in a view function in the Aggregator."

Medium criteria state that:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

This has not been shown.

[sherlock-admin]

Escalate for 10 USDC.

There is no impact of loss of funds shown in this issue. By the Sherlock criteria it does not qualify for a Medium.

Watson states as impact that "any parties (internal or external) that rely on the getMarketInfoForPurchase function will have an issue because of the inconsistent result returned by the interface." However this explanation is not concrete, does not include a POC, and does not show a scenario where loss of funds are possible.

Sponsor notes that the incorrect function is " only used in a view function in the Aggregator."

Medium criteria state that:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

This has not been shown.

You've created a valid escalation for 10 USDC!

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.

[Oighty]

Issue fixed here: https://github.com/Bond-Protocol/bonds/pull/48

[xiaoming9090]

Fixed in https://github.com/Bond-Protocol/bonds/pull/48

[hrishibhat]

Escalation accepted

Not a valid medium Given that this affects only the view function. Considering this issue a low

[sherlock-admin]

Escalation accepted

Not a valid medium Given that this affects only the view function. Considering this issue a low

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.