search

code-423n4 / 2022-12-prepo-findings

0 stars 1 forks source link

Hooks send funds to treasury without confirming that treasury is set #258

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L76 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/DepositHook.sol#L49 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/RedeemHook.sol#L21

Vulnerability details

Impact

In the withdraw(), deposit(), and redeem() functions, hooks are called that handle sending fees to the treasury.

Each of the withdrawHook, depositHook, and redeemHook contracts inherit from TokenSenderCaller, which has a _treasury variable and a setTreasury() function.

If the setTreasury() function isn't called for each of these hooks, _treasury will equal address(0).

Then, when the corresponding functions are called, fees will be sent to the zero address instead of the treasury and will be irretrievable.

Proof of Concept

The value of _treasury isn't set in the constructor or anywhere except the setTreasury() function.

If this value is accidentally not set, there is no check in the hook that it isn't address(0).

The result is that the following lines will send funds to the zero address:

collateral.getBaseToken().transferFrom(address(collateral), _treasury, _fee);

Tools Used

Manual Review

Recommended Mitigation Steps

Add a zero address check to each of these hooks:

require(_treasury != address(0), 'treasury address must be set');
Picodes commented 1 year ago

Low severity as it is a safety check, requires a configuration error and would only concern the fees

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-prepo-findings/issues/256