Hubble contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

By spamming withdrawal requests of 0, the user can clog the withdrawal queue. For anybody to withdraw their funds somebody has to first unclog it by running processWithdrawal(). Depending on the number of spam withdrawals, potentially multiple times.

Since the attacker doesn't have to burn any tokens they just have to pay the transaction fees. Since the project is supposed to be deployed to Avalanche that shouldn't be that expensive. Even if they can't withdraw 0 tokens the attack vector is still valid. They would simply deposit tokens before the attack and get them back after all their withdrawal requests were executed.

Since the creation of withdrawal requests is cheaper than executing the withdrawals it won't "hurt" the attacker as much as the attacked.

Proof of Concept

You simply create a contract that executes withdraw(0) a large number of times.

Anytime you want DOS the withdrawal process you trigger the contract again.

Tools Used

none

Recommended Mitigation Steps

I don't see the benefit of having a withdrawal queue. I'd say the withdraw() function should transfer the funds directly.

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L122 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L130 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L170 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L194 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L251 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L263 https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/ClearingHouse.sol#L277

Vulnerability details

Impact

Inside the ClearingHouse contract there are multiple instances where it loops over the dynamic amms array. There is no way looping over a subset of the array or continuing for you left off. Thus, if the array is large enough certain functions won't be executable anymore because of the block gas limit.

The approach is very prone to error and might easily result in a DOS.

see SWC-128

Proof of Concept

See linked code.

Tools Used

none

Recommended Mitigation Steps

Consider wrapping the logic the same way the withdrawal is handled in VUSD: https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/VUSD.sol#L53-L66

Or redesign the contract to work with individual AMM instances instead of looping over all of them.