Allowing msg.sedner to be onlyCollector leads to the msg.sender being able to collect all the fees for themselves.

Description

• In the constructor the _collector parameter can be set to any address, as long as its not a zero address. Then it gets set equal to s_collector, which then gets checked in the modifier onlyCollector to see if the address is the msg.sender. If not then it reverts.

• Now that the msg.sender is the onlyCollector they are able to call the function collectFee which will send all the weth from the contract to their address.

Risk

Likelihood: High

• this could happen anytime time since the constructor needs an address for the _collector parameter, which then gets equaled to s_collector, which then is used as a check in the modifier onlycollector to see if s_collector is the msg.sender and if it's not it will revert. But we can't have that modifier revert because without it we can't call essential functions in the contract like collectFee and changeCollector.

Impact: High

• This would lead to anyone being able to call the collectFee function and taking all the fees for themselves.

Proof of Concept

-Bob inputs his own address in the constructor parameters for _collector
-This parameter is then set equal to s_collector
-s_collector is then checked in the modifier onlyCollector to see if it's the address of msg.sender
-Now Bob gained access to the collectFee function since he's the onlyCollector
-Now Bob calls the function and transfers whatever fees have been accumulated to his address.

Recommended Mitigation

Instead of having the modifier revert if the s_collector isn't the msg.sender it should instead revert if it is the msg.sender.