code-423n4 / 2023-10-wildcat-findings

13 stars 10 forks source link

The lender should receive the over-collateralization asset when market is closed #89

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L157

Vulnerability details

Impact

Lenders are the ones who provide collateral and when closing the market, excess assets should be distributed back to them.

Proof of Concept

Two texts from https://code4rena.com/contests/2023-10-the-wildcat-protocol#top

  1. The borrower is not required to put any collateral down themselves when deploying a market, but rather there is a percentage of the supply, the reserve ratio, that must remain within the market.

  2. If you're wondering, 'wait, does that mean that lenders are collateralising their own loans?', the answer is yes, they absolutely are.

mentioned that the ones who provide collateral are the lenders and not the borrower, unlike other DEFI protocols.

Tools Used

Manual

Recommended Mitigation Steps

To distribute excess assets to lenders, first need to keep track of them. Below is a simplified modification to the WildcatMarket contract to do so:

// Add an array to keep track of all deposits
address[] public allDepositors;

// Add a mapping to check if an address has already deposited
mapping(address => bool) public hasDeposited;

// Modify the depositUpTo function to record all deposits
function depositUpTo(uint256 amount) public virtual nonReentrant returns (uint256 /* actualAmount */) {
  // ... (existing code)
  // Cache account data and revert if not authorized to deposit.
  lenders[msg.sender] += amount; // Existing line to record the lender's deposit

  if (!hasDeposited[msg.sender]) {
    allDepositors.push(msg.sender);
    hasDeposited[msg.sender] = true;
  }

  // ... (existing code)
  return amount;
}

// Modify the closeMarket function to distribute excess assets to lenders
function closeMarket() external onlyController nonReentrant {
  // ... (existing code)
  uint256 currentlyHeld = totalAssets();
  uint256 totalDebts = state.totalDebts();
  if (currentlyHeld > totalDebts) {
    uint256 excess = currentlyHeld - totalDebts;
    for (uint i = 0; i < allDepositors.length; i++) {
      address lenderAddress = allDepositors[i];
      uint256 lenderShare = (lenders[lenderAddress] * excess) / state.scaledTotalSupply;
      asset.safeTransfer(lenderAddress, lenderShare);
    }
  }
  // ... (existing code)
}

Assessed type

ERC20

code423n4 commented 11 months ago

Withdrawn by caventa