Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting a default prizeStrategy will save gas from avoiding zero-address checks #30

Closed
code423n4 opened this issue Jun 23, 2021 · 2 comments

Comments

@code423n4
Copy link
Contributor

Handle

0xRajeev

Vulnerability details

Impact

Given that PrizePools are required to be always associated with one prizeStrategy at any point, it should be possible to have a default one set in the initializer. The owner can always setPrizeStrategy() again to change the default if required, perhaps as part of the deployment to avoid any race-conditions.

Impact: This will avoid the zero-address checks (in beforeTokenTransfer and _mint) for prizeStrategy in PrizePool.sol and reduce gas usage from the expensive SLOAD storage reads. Removing the zero-address check will reduce 2 SLOADS of prizeStrategy to 1 thereby saving 100 gas for every depositTo and token transfer.

Proof of Concept

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

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

Tools Used

Manual Analysis

Recommended Mitigation Steps

Set prizeStrategy to a default (non-zero) one in the initializer which will allow the removal of zero-address checks.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Jun 23, 2021
code423n4 added a commit that referenced this issue Jun 23, 2021
@asselstine
Copy link
Collaborator

We explicitly want to allow a prize pool to disconnect a prize strategy so that users may withdraw in the event of a problem. The prize strategy may be zero.

@dmvt
Copy link
Collaborator

dmvt commented Jul 21, 2021

Sponsor's explanation makes sense

@dmvt dmvt closed this as completed Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants