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

1 stars 0 forks source link

Missing isShutdown on _checkpointAndClaim() #123

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

Fitraldys

Vulnerability details

Impact

Checking on isShutdown is happening on _checkpoint() to prevent checkpointing when there are problems occur, however check isShutdown is not happening on _checkpointAndClaim(), therefore when shutdown is happening checkpoint could also happening if a user call getReward().

Proof of Concept

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexStakingWrapper.sol#L279

iamsahu commented 2 years ago

shutdown is not implemented to allow users to claim their rewards if anything is left behind.

GalloDaSballo commented 2 years ago

The warden did a poor job of exploring what the finding entails.

@iamsahu it does seem like after running shutdownAndRescue some rewards may still be in the contract convexPool, it also seems like _checkpoint which can be called via user_checkpoint would not allow to claim the rewards from convexPool

However calling getReward which calls _checkpointAndClaim would allow to claim those outstanding rewards from convexPool

It seems like the finding has merit although underwhelmingly explored.

What do you think?

iamsahu commented 2 years ago

@GalloDaSballo We have deliberately not added shutdown to let users claim the rewards that are left behind in the contract as per their convenience. If we had added shutdown to _checkpointAndClaim we would had to transfer the rewards in the shutdownAndRescue and we had to distribute the rewards received individually later on. So, we had decided to not rescue the rewards using the shutdownAndRescue and let the user claim it at their convenience.

GalloDaSballo commented 2 years ago

Per the sponsors comment, shutdown will rescue the funds but rewards will still be claimable.

In lack of a POC, am marking the finding invalid