Open code423n4 opened 2 years ago
While I think the warden could have done a better job at explaining where, why and how to apply the nonReentrant
modifier, the sponsor has applied the improvement.
In spite of a lack of a specific attack vector, especially for the functions calling _checkpoint
and user_checkpoint
, due to an order that is inconsistent with checks-effect-interaction
, the modifier is a very welcome addition.
Because of a lack of a specific way to exploit the lack of the modifier, I believe Low Severity to be appropriate
Handle
Dravee
Vulnerability details
Impact
No protection from reentrancy (besides the gas limit on safeTransfer). Bad practice compared to the original
ConvexStakingWrapper
contract.Proof of Concept
The original
ConvexStakingWrapper
contract used thenonReentrant
modifier on all functions using thesafeTransfer
orsafeTransferFrom
methods:deposit
: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L337stake
: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L352withdraw
: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L367withdrawAndUnwrap
: https://github.com/convex-eth/platform/blob/main/contracts/contracts/wrappers/ConvexStakingWrapper.sol#L381As the current one in the Yield solution is an upgrade, it should follow the same good practices.
Tools Used
VS Code
Recommended Mitigation Steps
Use the
nonReentrant
modifier on external functions that end up callingsafeTransfer
orsafeTransferFrom
(user_checkpoint()
andgetReward()
)