code-423n4 / 2022-02-hubble-findings

2 stars 2 forks source link

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

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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;
}
atvanguard commented 2 years ago

Not an issue because reserveToken is intended to be USDC.

JasoonS commented 2 years ago

Not an issue because reserveToken is intended to be USDC.

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