prePO contest - deliriusz's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L72

Vulnerability details

Impact

WithdrawalHook::lastUserPeriodReset is global for all users, which means that each time that lastUserPeriodReset + userPeriodLength < block.timestamp, only one user will have a chance to update lastUserPeriodReset and reset theirs userToAmountWithdrawnThisPeriod. In extreme cases, either by malicious user, or high amount of withdrawals spread accross long time, a user that reached userToAmountWithdrawnThisPeriod in one period, would suffer tokens locked accross multiple periods. In an edge case, malicious user can DoS others for days, weeks or even months, depending on their will, as minimum withdrawal is relatively small.

Proof of Concept

Proof of concept is prepared based on the test files: Drop following code in WithdrawHook.test.js under line 372 and add line await provider.send("evm_mine", []) in smart-contracts-utils.ts::setNextTimestamp()`

      describe('# exploitsUserWithdrawal', () => {

        it('is able to block user from withdrawal', async () => {
          let previousResetTimestamp = await getLastTimestamp(ethers.provider)
          // actually this function does not work properly, as increasing timestamp has to be followed by:
          // await provider.send("evm_mine", [])
          // Otherwise the timestamp doesn't increase. I added it in the utils file for the test to work properly

          await setNextTimestamp(
            ethers.provider,
            previousResetTimestamp + TEST_USER_PERIOD_LENGTH - 1
          )

          const secondUserWithdrawLimit = TEST_USER_WITHDRAW_LIMIT.div(3);
          //This is a prerequisite for this issue - user has to reach limit in one withdrawal period
          await withdrawHook
            .connect(collateralSigner)
            .hook(user.address, TEST_USER_WITHDRAW_LIMIT, TEST_USER_WITHDRAW_LIMIT)
          await withdrawHook
            .connect(collateralSigner)
            .hook(collateralSigner.address, secondUserWithdrawLimit, secondUserWithdrawLimit)
          expect(await withdrawHook.getAmountWithdrawnThisPeriod(user.address)).to.eq(
            TEST_USER_WITHDRAW_LIMIT
          )

          expect(await withdrawHook.getAmountWithdrawnThisPeriod(collateralSigner.address)).to.eq(
            secondUserWithdrawLimit
          )

          await expect(
            withdrawHook.connect(collateralSigner).hook(user.address, 100, 100)
          ).to.revertedWith('user withdraw limit exceeded')

          previousResetTimestamp = await getLastTimestamp(ethers.provider)
          await setNextTimestamp(
            ethers.provider,
            previousResetTimestamp + TEST_USER_PERIOD_LENGTH + 1
          )


          await expect(
            withdrawHook.connect(collateralSigner).hook(collateralSigner.address, 1, 1)
          ).to.not.be.reverted

          await expect(
            withdrawHook.connect(collateralSigner).hook(user.address, 1, 1)
          ).to.revertedWith('user withdraw limit exceeded')

          //Now some time passes, and all users should have their personal limits cleared
          previousResetTimestamp = await getLastTimestamp(ethers.provider)
          await setNextTimestamp(
            ethers.provider,
            previousResetTimestamp + TEST_USER_PERIOD_LENGTH + 1
          )

          await expect(
            withdrawHook.connect(collateralSigner).hook(collateralSigner.address, 1, 1)
          ).to.not.be.reverted

          expect(await withdrawHook.getAmountWithdrawnThisPeriod(collateralSigner.address)).to.eq(1)

          //However because the user was not the first in the block, his limit persists
          await expect(
            withdrawHook.connect(collateralSigner).hook(user.address, 1, 1)
          ).to.revertedWith('user withdraw limit exceeded')

          // As long as the user won't be the first to withdraw after new period starts, his 
          // current limit will persist, which can lead to DoS for very long time by a malicious user
          expect(await withdrawHook.getAmountWithdrawnThisPeriod(user.address)).to.eq(
            TEST_USER_WITHDRAW_LIMIT
          )
        })
      })

Tools Used

VS Code, hardhat

Recommended Mitigation Steps

I see two options here:

1. Remove lastUserPeriodReset at all, as tracking global withdrawals seems to be more important
2. Trace lastUserPeriodReset per each user separately

Probably the first option is better, as single user withdrawal may not be such an issue compared to the global withdrawal, and tracking it per user basis would increase gas usage.

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L58

Vulnerability details

Impact

Manager for Collateral smart contract has complete role over collateral funds and is able to withdraw all of it

The assumption is that the manager is honest, even though there is a big risk with wrongly configured MultiSig (e.g. admin that can circumvent M of N votes to take an action), keys compromise, even moving funds to unepected address which would lead to funds being lost.

Proof of Concept

Manager can call managerWithdraw to get all the funds:

80:  function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
81:    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);
82:    baseToken.transfer(manager, _amount);
83:  }

or halt withdrawals:

Colateral.sol
73:  withdrawHook.hook(msg.sender, _baseTokenAmount, _baseTokenAmountAfterFee);

WithdrawalHook.sol
//withdrawalsAllowed is set by manager account 
58:    require(withdrawalsAllowed, "withdrawals not allowed");//@audit-issue LOW - by default it's false, any withdrawal will fail

Tools Used

VS Code

Recommended Mitigation Steps

If only function of this is to move funds to some yield bearing protocol (e.g. Aave, DEXes), please implement supposed functionality in the smart contract. Otherwise, it's best to remove the function completely.

[L-01] No constructor / uninitialized constructor for upgradeable contract

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L29 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L16

While the smart contracts in scope of this issue are initialized in proxy contract, leaving them uninitalized may lead to unexpected behaviours, as anyone may initialize them. Best practise in this case is addimg "initializer" midifier to the constructor to disallow that. Please reffer to the documentation: https://docs.openzeppelin.com/contracts/4.x/api/proxy#TransparentUpgradeableProxy

" ...An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, which may impact the proxy... "

[L-02] No storage gap set for upgradeable smart contract

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L28 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L15

Even though the smart contracts in scope are not being inherited by any smart contract, it's a good practise to create uint256[50 - <maxStorageLot>] __gap;, hwich prevents storage clashing in proxy, in case that the contract was to be inherited from in the future.

Please reffer to https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps for details.

[L-04] If depositsAllowed is false && Collateral address is set, it will brick deposits

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L53 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositHook.sol#L44 if depositsAllowed is false && Collateral address is set, it will brick deposits

[L-05] __gap + storage slots should sum up to 50 in upgradeable contracts

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/packages/prepo-shared-contracts/contracts/SafeAccessControlEnumerableUpgradeable.sol#L65 Even though this contract is not in scope, some contracts that are in scope inherit from it and are directly impacted by this issue. All storage slots used with __gap array elements count should sum up to constant number, usually 50 - not updating it whenever storage layout changes may shift whole storage layout and lead to unxepected behaviour. And there are 2 storage slots already used by mappings in this contract, which together with the gap sum up to 52 slots. Actually, documentation quoted in the comment above the __gap says just that:

The size of the __gap array is calculated so that the amount of storage used by a contract always adds up to the same number (in this case 50 storage slots).

[NC-01] Avoid using magic numbers

    uint256 _collateralMintAmount = (_amountAfterFee * 1e18) / baseTokenDenominator;
    uint256 _baseTokenAmount = (_amount * baseTokenDenominator) / 1e18;

[NC-02] use most recent Solidity version

This issue concerns all the contracts in scope

[NC-03] Unresolved TODOs

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/TokenSender.sol#L15

15:  WithdrawERC20, // TODO: Access control when WithdrawERC20 updated

[NC-04] Collateral code doesn't match documentation

According to docs https://docs.prepo.io/concepts/prect : "preCT is backed by a basket of yield bearing USD stablecoins", while Collateral allows only for one token. Documentation should be probably updated to reflect that.