code-423n4 / 2021-09-wildcredit-findings

0 stars 0 forks source link

Missing event for this critical onlyOperator function where the operator can arbitrarily change name+symbol #74

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

All LP tokens for all lending pairs are created with the same name and symbol of “WILD-LP” while requiring the lendingController.owner to update them individually. The motivation for this is captured in the code comment: “// LP tokens can be created by anyone. Some tokens are not suitable for automated naming. // This function allow the operator to set a unique name for each LP token.”

While this approach itself is not scalable and requires the operator to keep track of new pairs and renaming all of them to prevent users from being mislead, it is made riskier by not emitting an event (and not enforcing a timelock). Missing event for this critical onlyOperator function where the operator can arbitrarily change name+symbol makes this susceptible to operator risk where a malicious/compromised/negligent operator can change the name/symbol to arbitrary values. Without an event+timelock, user interfaces and monitoring tools will be confused when this happens and can lead to major confusion/outages/panic.

Proof of Concept

https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LPTokenMaster.sol#L43-L48

https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LPTokenMaster.sol#L38-L39

https://github.com/code-423n4/2021-09-wildcredit/blob/c48235289a25b2134bb16530185483e8c85507f8/contracts/LPTokenMaster.sol#L28-L31

Tools Used

Manual Analysis

Recommended Mitigation Steps

Evaluate a more scalable and transparent approach using which LP names can be tracked and made as unique as possible without putting the burden on users/operators to perform this due-diligence.

talegift commented 3 years ago

There is nothing in the code to suggest that it will or will not be under a Timelock.

The operator will be the owner of LendingController which will be a TimeLock.

The primary concern of this issue seems to be user confusion. I don't see that as critical.

Agreed that adding an event is a good idea. However, since it doesn't impact funds or even the state of any balance-related variables in any way, the severity should be 0.

ghoul-sol commented 3 years ago

This is a best practices recommendation. Non-critical.