Evaluate security benefit vs gas usage trade-off for using nonreentrant modifier on different functions

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

While it may be considered extra-safe to have a nonreentrant modifier on all functions making any external calls even though they are to trusted contracts, the only calls are transfers not low-level calls and functions implement Checks-Effects-Interactions (CEI) pattern, it is helpful to evaluate the perceived security benefit vs gas usage trade-off for using nonreentrant modifier.

Functions: 1) Not making external low-level calls (transfers are reentrancy safe due to 2300 gas subsidy) to untrusted (non-owner deployed or unknown yield sources) contracts or could be potential destinations of such reentrant calls and 2) Adhering to the CEI pattern — may consider not having the nonreentrant modifier which does two SSTORES (getting more expensive with the London fork EIP-3529) to its _status state variable.

Impact: Save gas by removing nonreentrant modifier if function is deemed to be reentrant safe. This can save gas costs of 2 SSTORES per function call that uses this modifier: _status SSTORE from 1 to 2 costs 5000 and _status SSTORE from 2 to 1 which costs 100 because it was already accessed) which is significant at 5100 per call post-Berlin EIP-2929.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4d0f8c1da8654a478f046ea7cf83d2166e1025af/contracts/security/ReentrancyGuard.sol#L36

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4d0f8c1da8654a478f046ea7cf83d2166e1025af/contracts/security/ReentrancyGuard.sol#L49-L61

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L277

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L301

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L326

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L377

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L450

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/PrizePool.sol#L630

and many others that use this modifier in the different yield source contracts

Tools Used

Manual Analysis

Recommended Mitigation Steps

Evaluate security benefit vs gas usage trade-off for using nonreentrant modifier on functions that may already be reentrant safe or do not need this protection. It may indeed be safe to leave this modifier (while accepting the gas impact) if such an evaluation is tricky or depends on assumptions.

[asselstine]

Yes; we did analyze the code and that's why we added reentrancy modifiers.

This issue is hand waving that there may be an optimization.

[dmvt]

I agree with the sponsor. No specific removals were recommended here.