Skip to content
This repository has been archived by the owner on Dec 31, 2023. It is now read-only.

bitsurfer - Wrong assignment of cumulativeBid for RangeOrder state in getRangeOrderState function #178

Open
sherlock-admin opened this issue Jul 1, 2023 · 2 comments
Labels
Medium A valid Medium severity issue Reward A payout will be made for this issue Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

bitsurfer

high

Wrong assignment of cumulativeBid for RangeOrder state in getRangeOrderState function

Summary

Wrong assignment of cumulativeBid for RangeOrder state

Vulnerability Detail

In D3Trading, the getRangeOrderState function is returning RangeOrder (get swap status for internal swap) which is assinging wrong toTokenMMInfo.cumulativeBid which suppose to be cumulativeBid not cumulativeAsk

The error lies in the assignment of roState.toTokenMMInfo.cumulativeBid. Instead of assigning tokenCumMap[toToken].cumulativeAsk, it should be assigning tokenCumMap[toToken].cumulativeBid.

File: D3Trading.sol
86:         roState.toTokenMMInfo.cumulativeBid =
87:             allFlag >> (toTokenIndex) & 1 == 0 ? 0 : tokenCumMap[toToken].cumulativeAsk;

This wrong assignment value definitely will mess up accounting balance, resulting unknown state will occure, which is not expected by the protocol

For one case, this getRangeOrderState is being used in querySellTokens & queryBuyTokens which may later called from sellToken and buyToken. The issue is when calling _contructTokenState which can be reverted from PMMRangeOrder when buy or sell token

File: PMMRangeOrder.sol
100:         // B
101:         tokenState.B = askOrNot ? tokenState.B0 - tokenMMInfo.cumulativeAsk : tokenState.B0 - tokenMMInfo.cumulativeBid;

When the tokenMMInfo.cumulativeBid (which was wrongly assign from cumulativeAsk) is bigger than tokenState.B0, this will revert

Impact

This wrong assignment value definitely will mess up accounting balance, resulting unknown state will occure, which is not expected by the protocol. For example reverting state showing a case above.

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Pool/D3Trading.sol#L86-L87

Tool used

Manual Review

Recommendation

Fix the error to

File: D3Trading.sol
86:         roState.toTokenMMInfo.cumulativeBid =
--:             allFlag >> (toTokenIndex) & 1 == 0 ? 0 : tokenCumMap[toToken].cumulativeAsk;
++:             allFlag >> (toTokenIndex) & 1 == 0 ? 0 : tokenCumMap[toToken].cumulativeBid;
@github-actions github-actions bot added the Medium A valid Medium severity issue label Jul 5, 2023
@Attens1423 Attens1423 added the Will Fix The sponsor confirmed this issue will be fixed label Jul 18, 2023
@Attens1423
Copy link

@sherlock-admin2 sherlock-admin2 added the Reward A payout will be made for this issue label Jul 24, 2023
@IAm0x52
Copy link
Collaborator

IAm0x52 commented Sep 6, 2023

Fix looks good. getRangeOrderState now correctly updates cumulativeBid instead of cumulativeAsk twice

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Medium A valid Medium severity issue Reward A payout will be made for this issue Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants