spyrosonic10 - Possibility of underflow in calling `initiateBatchAuction`

[sherlock-admin]

spyrosonic10

medium

Possibility of underflow in calling initiateBatchAuction

Summary

BondBatchAuctionV1.sol has a function amountWithFee() which calculate amount with fee from given input and this function has a logic to check fee like this _teller.protocolFee() - _teller.createFeeDiscount(). There is no check to make sure protocolFee is higher than createFeeDiscount and hence has potential to fail if it is lower.

Vulnerability Detail

There are multiple instance where code does check if (protocolFee > createFeeDiscount) which indicate that it is possible for protocolFee to be lower than createFeeDiscount and in such cases failure will occur. Fee and discount are set in BondBaseTeller(which is not in scope) and it does suggest that there is no safe check in place during setting setProtocolFee which also validate possible existence of vulnerability.

Impact

If protocolFee is lower than createFeeDiscount then amountWithFee will fail and which cause initiateBatchAuction to fail.

Code Snippet

https://github.com/sherlock-audit/2023-02-bond/blob/main/bonds/src/BondBatchAuctionV1.sol#L306-L321

Tool used

Manual Review

Recommendation

Consider adding if (protocolFee > createFeeDiscount) before performing subtraction.

[Oighty]

It's not supposed to be able to be greater. We do check that createFeeDiscount is not set to be greater than protocolFee in the setCreateFeeDiscount function on the BaseTeller, but I see that we didn't include a check on the setProtocolFee function to ensure it is greater than createFeeDiscount. Therefore, you could technically set it higher by setting in and then changing the protocolFee. Based on that, I agree it makes sense to add this check, but since it is a protocol parameter, the risk is low.

[spyrosonic10]

Escalate for 10 USDC In my opinion this issue is NOT a duplicate of #36 . While I agree both issues impacts initiateBatchAuction but they are different. Root cause are different and the fixes are different too. My issue is about missing check for protocolFee while the other one is missing check for liquidityAmount.

Edit: One more observation, #36 is marked as Sponsor Disputed and also marked as Reward. My general understanding is that Sponsor Disputed should not get rewards and that pie should be redistributed.

[sherlock-admin]

Escalate for 10 USDC In my opinion this issue is NOT a duplicate of #36 . While I agree both issues impacts initiateBatchAuction but they are different. Root cause are different and the fixes are different too. My issue is about missing check for protocolFee while the other one is missing check for liquidityAmount.

Edit: One more observation, #36 is marked as Sponsor Disputed and also marked as Reward. My general understanding is that Sponsor Disputed should not get rewards and that pie should be redistributed.

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.

[UsmannK]

Escalate for 10 USDC

This issue is false, it is not a dup of #36.

Watson reported that initiating a batch auction fails under the condition protocolFee < createFeeDiscount. Sponsor says this is intended. Sponsor goes on to add even more places where the condition protocolFee < createFeeDiscount causes reverts.

The revert observed in the report is intended behavior.

[sherlock-admin]

Escalate for 10 USDC

This issue is false, it is not a dup of #36.

Watson reported that initiating a batch auction fails under the condition protocolFee < createFeeDiscount. Sponsor says this is intended. Sponsor goes on to add even more places where the condition protocolFee < createFeeDiscount causes reverts.

The revert observed in the report is intended behavior.

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/51

[hrishibhat]

Escalation accepted

Not a valid duplicate of #36 Also considering this issue as non-valid since the code is working as intended assuming the teller configurations are correct. Teller configurations are admin controlled. Hence this is not a valid medium/high

[sherlock-admin]

Escalation accepted

Not a valid duplicate of #36 Also considering this issue as non-valid since the code is working as intended

This issue's escalations have been accepted!

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

[xiaoming9090]

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