code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/DexManagerFacet.sol#L55

Vulnerability details

Impact

When batchRemoveDex() function is called with an array of dexes to be removed, the first dex in the _dexs array would be removed, but then the inner for loop would return from the function without removing the rest of the dexes. Hence, the rest of the dexes would still be whitelisted and operational.

Proof of Concept

1- Add 2 dexes using batchAddDex(): 2- Call approvedDexs(). It will return the 2 added adresses: address[]: 0xDEF171Fe48CF0115B1d80b88dc8eAB59176FEe57,0x216B4B4Ba9F3e719726886d34a177484278Bfcae 3- Call batchRemoveDex() passing this address array as argument 4- Call approvedDexs() again. It will return 1 adress whereas there should be none: address[]: 0x216B4B4Ba9F3e719726886d34a177484278Bfcae

Tools Used

Remix

Recommended Mitigation Steps

Replace the return statement on line 55 with break, so that only the inner loop would break, but the outer loop will continue execution.

H3xept commented 2 years ago

Duplicate of #34

gzeoneth commented 2 years ago

Changing to Med Risk to align with #34

code-423n4 / 2022-03-lifinance-findings