yixxas - removeAdaptor() is not removing the removed money market from withdrawSeq[].

[sherlock-admin]

yixxas

medium

removeAdaptor() is not removing the removed money market from withdrawSeq[].

Summary

Removing an adaptor should also remove the corresponding element in withdrawSeq[], but is not done so in the current implementation.

Vulnerability Detail

withdrawSeq[] is used in the withdraw() function in AssetManager.sol. It iterates over the withdrawSeq[] array and it would still contain the removed money market. This can lead to unexpected interactions as it still tries to withdraw from the already removed money market. Furthermore, the size of withdrawSeq[] cannot be reduced. setWithdrawSequence() only allows changing the order of the indexes, but length must remain the same. This can lead to confusions when an admin tries to change the order of the sequence, but fails as withdrawSeq.length > moneyMarkets.length.

Impact

Unexpected interactions with a removed money market can lead to unforeseen problems and causing confusions due to how withdrawSeq[] size would always be higher than moneyMarkets[] size which cannot be fixed. I believe this is an oversight by the protocol developer team.

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L440-L457
https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L139-L142
https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L348-L359

Tool used

Manual Review

Recommendation

Add the logic of removing corresponding index from withdrawSeq[] when removeAdaptor() is called.

Duplicate of #76