search

code-423n4 / 2022-01-sherlock-findings

0 stars 0 forks source link

Users shouldn't be forced into a specific strategy (possible rug pull) #204

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

harleythedog

Vulnerability details

Impact

As already discussed in the previous Sherlock C4 contest here, it is best to mitigate rug pull possibilities (even if the team is well intentioned, there is still the risk of being called out, and less users might interact with the project if the funds can technically be rugged).

Now, in Sherlock.sol, the functions updateYieldStrategy and yieldStrategyDeposit can be called even when the protocol is paused. This means that the owners can pause the protocol (so that withdraws are not allowed), set the strategy to a malicious contract, and then transfer all of the funds to this malicious strategy. Assuming the protocol can become paused in a short period of time, a timelock doesn't mitigate this issue either since users can't withdraw even if they see this rug pull coming (since the protocol is paused). To promote trust and mitigate this issue, there should be a minimum amount of time where a user can withdraw their funds from Sherlock before a new strategy is set.

Proof of Concept

See updateYieldStrategy here and see yieldStrategyDeposit here.

Tools Used

Inspection.

Recommended Mitigation Steps

I recommend adding some sort of mechanism for users to be able to react to new strategy changes. For example, a new strategy is proposed and users get 5 days to withdraw if they want. If the protocol is paused/becomes paused, then the countdown is reset until unpausing since it would be unfair for users to be forced into a strategy while they aren't able to withdraw.

Evert0x commented 2 years ago

Non-critical

I agree this allows for rug pulls. Eventually we want to move to an owner contract that has timelocks for yieldStrategyUpdates and doesn't allow calls to pause and unpause. I think this will be in line with your comments

jack-the-pug commented 2 years ago

https://github.com/code-423n4/2022-01-sherlock/blob/c763f10c4b5fe2127677d6c25b83adcf3bcec212/scripts/2_timelock.js#L45-L46

  await (await sherlock.transferOwnership(timelock.address)).wait();
  console.log('2 - Transferred sherlock ownership');

Sharlock.sol is already owned by a timlock, no?