sellMalt(), addLiquidity() and removeLiquidity() Allow Non Privileged Users Withdraw Fund

[code423n4]

Handle

Meta0xNull

Vulnerability details

Impact

Is Not Uncommon Normal Users Accidentally Send Tokens into Contract.

ENS Airdrop is a Good Example Normal Users Accidentally Send Tokens into Contract:
https://discuss.ens.domains/t/social-amend-airdrop-proposal-to-include-accidentally-returned-funds/6975

In UniswapHandler.sol, sellMalt(), addLiquidity() and removeLiquidity() Have No Access Control. When Normal Users Accidently Deposit Tokens into the Contract, Any Random Persons/Bot Can Withdraw the Tokens because it will safeTransfer to msg.sender who find out there is token balance in the contract.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L185-L219
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L221-L245

Tools Used

Manual Review

Recommended Mitigation Steps

Add relevant access control, probably Only StabilizerNode and Admin have Access to this contract various functions like sellMalt(), addLiquidity() and removeLiquidity() etc.

[0xScotch]

Disagree with severity as there is no part of the protocol that would leave funds in this contract. Therefore there is no direct risk of protocol user's funds. This can only lead to lost funds if a random user sends funds directly to this contract.

[GalloDaSballo]

As the Sponsor said, the contract won't have any additional funds.
As such there's no particular risk for funds.
Am going to mark this as low because there seems to be an inconsistency (lack of clarity) between the buyMalt and the sellMalt, I believe allowing both to be called by anyone is completely fine as the contract is just a utility tool