Centralization risk - owner can perform DoS on users which have deposited and/or can withdraw their funds.

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L53 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L73 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L102 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L107 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L96

Vulnerability details

Impact

The hooks for deposit and withdraw in Collateral.sol, and mint and redeem in PrePOMarket.sol don't have a timelock or limitation to what contract addresses they're set. That means, if they are set to a contract which reverts on hook being called, all of users funds will be locked in the contract - whether Collateral.sol or PrePOMarket.sol.

Additionally, if the depositHook in Collateral.sol is set to address 0x0, the entire user's deposit will be sent to the contract on calling deposit. The deposit can subsequently be withdrawn by calling managerWithdraw from the MANAGER_WITHDRAW_ROLE address. This can be used to drain users funds after depositing.

Proof of Concept

1. DoS of withdraw function in Collateral.sol Deploy a new instance of the PrePO contracts as in the test fixtures. Deploy a smart contract with only one function - hook, which may look like this:

   contract DoS {
   function hook() public {
       require(false);
   }
   }

   User deposits some collateral by calling deposit(address _recipient, uint256 _amount) in Collateral.sol Call setWithdrawHook from the SET_WITHDRAW_HOOK_ROLE with _newWithdrawHook set to the address of the DoS contract. -> User cannot call withdraw, effectively trapping his funds in the contract.

2. DoS of redeem function in PrePOMarket.sol Deploy a new instance of the PrePO contracts as in the test fixtures. Deploy a smart contract with only one function - hook, which may look like this:

   contract DoS {
   function hook() public {
       require(false);
   }
   }

   User mints some long and short tokens by calling mint(uint256 _amount) in PrePOMarket.sol Call setRedeemHook from the owner of the contract with redeemHook set to the address of the DoS contract. -> User cannot call redeem, effectively trapping his funds in the contract.

3. Withdrawing user's funds from Collateral.sol Deploy a new instance of the PrePO contracts as in the test fixtures. Call setRedeemHook from the SET_WITHDRAW_HOOK_ROLE role with redeemHook set to the address address(0). User deposits some collateral by calling deposit(address _recipient, uint256 _amount). Call managerWithdraw from the MANAGER_WITHDRAW_ROLE to withdraw the tokens in the contract.

Tools Used

Forge, VS Code Plugins

Recommended Mitigation Steps

Deploy DepositHook, WithdrawHook, MintHook, RedeemHook from a Deployer (Factory) contract, which will keep their implementations in storage, and update the hooks addresses in Collateral and PrePOMarket contracts. Remove set* functions. That way, the contracts will be the same contracts as the hooks in the repo, and it wouldn't be possible for the hooks to be set to a malicious contract. Another way to mitigate this is to add a Timelock contract, which will set a timelock on the new hook address to be upgraded - for example, upgrade in 3 days. That way the users will have more time to withdraw in case there's any malicious activity on admin's end.

[hansfriese]

duplicate of #254

[c4-judge]

Picodes marked the issue as duplicate of #305

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b