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

Blocking of the VUSD withdrawals is possible if the reserve token doesn't support zero value transfers #29

Open
code423n4 opened this issue Feb 20, 2022 · 2 comments
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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L62

Vulnerability details

Impact

VUSD withdraw queue will be blocked and user funds frozen simply by requesting a zero value withdraw, if the reserve token doesn't support zero value transfers.

Putting it medium only on an assumption that reserve will be USDC and the probability is low, but VUSD do allow any reserve token and the impact here is both funds freeze and stopping of the operations

Proof of Concept

It is possible to burn zero amount in OZ implementation:

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L285-L300

So, withdraw will burn zero amount and put it to the queue:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L48

USDC does support zero value transfers, but not all the tokens do:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

Currently VUSD can use any reserve token:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L33

Withdraw queue position can be modified in the processWithdrawals function only.

But it will fail every time on the zero amount entry, as there is no way to skip it (and mint VUSD back, for example), so anything else after this zero entry will not be processed:

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L62

This way the withdrawal functionality and the corresponding user funds will be frozen within VUSD contract, which will become inoperable

Recommended Mitigation Steps

Consider adding a zero amount check, as it doesn’t cost much, while zero transfer doesn't make sense anyway.

Now:

reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
reserve -= withdrawal.amount;

To be:

if (withdrawal.amount > 0) {
	reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
	reserve -= withdrawal.amount;
}
@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 Feb 20, 2022
code423n4 added a commit that referenced this issue Feb 20, 2022
@atvanguard
Copy link
Collaborator

Not an issue because reserveToken is intended to be USDC.

@atvanguard atvanguard added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Feb 24, 2022
@JasoonS
Copy link
Collaborator

JasoonS commented Mar 6, 2022

Not an issue because reserveToken is intended to be USDC.

Not specified in spec for the audit. Giving to submitter.

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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants