code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositRecord.sol#L28-L33 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositRecord.sol#L35-L38

Vulnerability details

Impact

In contract DepositRecord when a deposit is recorded through function recordDeposit:

  function recordDeposit(address _sender, uint256 _amount) external override onlyAllowedHooks {
    require(_amount + globalNetDepositAmount <= globalNetDepositCap, "Global deposit cap exceeded");
    require(_amount + userToDeposits[_sender] <= userDepositCap, "User deposit cap exceeded");
    globalNetDepositAmount += _amount;
    userToDeposits[_sender] += _amount;
  }

The global deposit amount is incremented and also the depositor's amount. The function checks that both the global and the per user caps are not surpassed. On the other hand, function recordWithdrawal:

  function recordWithdrawal(uint256 _amount) external override onlyAllowedHooks {
    if (globalNetDepositAmount > _amount) { globalNetDepositAmount -= _amount; }
    else { globalNetDepositAmount = 0; }
  }

Only decreases the global amount. Both functions are called from hooks when depositing/withdrawing funds. This means that eventually, a user can reach the cap and every deposit after that would be impossible unless the owner increases the cap, which is a variable that applies to all users.

Proof of Concept

To make it simple, imagine the following:

userDepositCap is set to 1,000
Alice deposits 500 tokens. userToDeposits[alice] = 500
Alice withdraws 500 tokens.
Alice deposits 500 tokens more. userToDeposits[alice] = 1,000
Alice withdraws 500 tokens.
Alice deposits 500 tokens more. The call reverts. Although Alice's collateral balance is zero, she cannot make any deposit.

Tools Used

Manual review

Recommended Mitigation Steps

Change function recordWithdrawal function:

function recordWithdrawal(address _sender, uint256 _amount) external override onlyAllowedHooks {
    if (globalNetDepositAmount > _amount) { globalNetDepositAmount -= _amount; }
    else { globalNetDepositAmount = 0; }
    if (userToDeposits[_sender] > _amount) { userToDeposits[_sender] -= _amount; }
    else { userToDeposits[_sender] = 0; }
  }

And change accordingly the contract WithdrawalHook, which calls recordWithdrawal.

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #294