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

PuttyV2.withdraw() might revert with zero transfer. #116

Closed
code423n4 opened this issue Jul 2, 2022 · 1 comment
Closed

PuttyV2.withdraw() might revert with zero transfer. #116

code423n4 opened this issue Jul 2, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L499-L500

Vulnerability details

Impact

PuttyV2.withdraw() might revert with zero transfer.
So users can't withdraw their funds and tokens.

Proof of Concept

As we can check from previous issue, some ERC20 tokens don't allow transfer of 0 amount.
And when we calculate feeAmount, feeAmount might be zero even though the fee is positive when order.strike is less than 1000.
Then the fee transfer might revert and users can't withdraw their funds and tokens.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

We can modify this part like below.

uint256 feeAmount = (order.strike * fee) / 1000;
if (feeAmount != 0) {
    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
}
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Jul 2, 2022
code423n4 added a commit that referenced this issue Jul 2, 2022
@outdoteth
Copy link
Collaborator

Duplicate: withdraw() can be DOS’d for baseAsset ERC20s that prevent 0 transfers if the calculated feeAmount is 0 due to rounding: #283

@outdoteth outdoteth added the duplicate This issue or pull request already exists label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants