code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

Malicious `VaultBooster` owner can steal others user deposited funds #148

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L142 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L171 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L188

Vulnerability details

Impact

Anyone can create a VaultBooster using the VaultBoosterFactory, then the owner sets a configuration to a specific token.

The problem is that the owner can change the _boosts[_token] data even when the data has been configured. Please see the next scenario:

  1. The owner at the beginning configure, using the setBoost() function, a legitimate liquidation pair and legitimate target vault.
  2. Users deposits to the VaultBooster because the user thinks the liquidation pair will introduce prize tokens so the configured vault has more chances to win.
  3. The malicious owner changes the liquidation pair to address(0) using the setBoost() function.
  4. Now, the liquidate function is not possible to be called because the liquidation pair is address(0).
  5. The owner withdraws the deposited funds by others users.

The malicious VaultBooster owner can steal funds from others.

Proof of Concept

The setBoost() function allows the owner to modify the _boosts[_token] parameters at any time. So the malicious owner can change the liquidation pair to zero address making to be unable to liquidate the users's deposited funds by a legitimate vault.

File: VaultBooster.sol
142:   function setBoost(IERC20 _token, address _liquidationPair, UD2x18 _multiplierOfTotalSupplyPerSecond, uint96 _tokensPerSecond, uint144 _initialAvailable) external onlyOwner {
143:     if (_initialAvailable > 0) {
144:       uint256 balance = _token.balanceOf(address(this));
145:       if (balance < _initialAvailable) {
146:         revert InitialAvailableExceedsBalance(_initialAvailable, balance);
147:       }
148:     }
149:     _boosts[_token] = Boost({
150:       liquidationPair: _liquidationPair,
151:       multiplierOfTotalSupplyPerSecond: _multiplierOfTotalSupplyPerSecond,
152:       tokensPerSecond: _tokensPerSecond,
153:       available: _initialAvailable,
154:       lastAccruedAt: uint48(block.timestamp)
155:     });
156: 
157:     emit SetBoost(
158:       _token,
159:       _liquidationPair,
160:       _multiplierOfTotalSupplyPerSecond,
161:       _tokensPerSecond,
162:       _initialAvailable,
163:       uint48(block.timestamp)
164:     );
165:   }

Then the malicious owner can withdraw users's deposited funds:

File: VaultBooster.sol
188:   function withdraw(IERC20 _token, uint256 _amount) external onlyOwner {
189:     uint256 availableBoost = _accrue(_token);
190:     uint256 availableBalance = _token.balanceOf(address(this));
191:     uint256 remainingBalance = availableBalance - _amount;
192:     _boosts[IERC20(_token)].available = (availableBoost > remainingBalance ? remainingBalance : availableBoost).toUint144();
193:     _token.transfer(msg.sender, _amount);
194: 
195:     emit Withdrawn(_token, msg.sender, _amount);
196:   }

The deposit to the VaultBooster can be by anyone:

File: VaultBooster.sol
171:   function deposit(IERC20 _token, uint256 _amount) external {
172:     _accrue(_token);
173:     _token.safeTransferFrom(msg.sender, address(this), _amount);
174: 
175:     emit Deposited(_token, msg.sender, _amount);
176:   }

Tools used

Manual review

Recommended Mitigation Steps

Add a validation that the _boosts[_token] can not be changed by the owner once the boosts[token] is configured.

Assessed type

Invalid Validation

raymondfam commented 1 year ago

A known centralized issue in the bot.

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #69

c4-pre-sort commented 1 year ago

raymondfam marked the issue as remove high or low quality report

c4-judge commented 1 year ago

HickupHH3 changed the severity to QA (Quality Assurance)