search

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

14 stars 10 forks source link

Borrower can steal all blocked lender's underlying assets #630

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L79 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L173-L174 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L178 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L96-L97 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L108 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L110 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L165 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L167-L168 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L171 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L38

Vulnerability details

The WildcatMarketConfig::nukeFromOrbit() and the WildcatMarketWithdrawals::executeWithdrawal() create incorrect Escrows for locking the blocked lender's asset tokens (underlying assets) and market tokens.

Impact

A borrower can steal all asset tokens and market tokens of the blocked lender. Eventually, the borrower can burn the stolen market tokens and withdraw all underlying asset tokens from the market.

Proof of Concept

This PoC section is divided into three sub-sections:

Explaining how the nukeFromOrbit() creates an incorrect Escrow for the market tokens

Once a lender gets sanctioned by Chainalysis, anyone can trigger the WildcatMarketConfig::nukeFromOrbit() to block the lender from interacting with the market and transfer all lender's market tokens to an Escrow contract.

The nukeFromOrbit() invokes the WildcatMarketBase::_blockAccount() to block the lender. The _blockAccount() will create the Escrow for holding the blocked lender's market tokens. However, the _blockAccount() will pass the accountAddress and borrower arguments into the WildcatSanctionsSentinel::createEscrow() alternately with the createEscrow()'s required parameters.

Subsequently, the createEscrow()'s borrower parameter will point to the blocked lender, whereas the account parameter will point to the borrower instead. Therefore, the createEscrow() will create the incorrect Escrow contract.

After that, all blocked lender's market tokens will be transferred to the incorrectly generated Escrow, allowing the borrower to execute the WildcatSanctionsEscrow::releaseEscrow() on the Escrow to steal all market tokens.

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketConfig.sol
    function nukeFromOrbit(address accountAddress) external nonReentrant {
      if (!IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
        revert BadLaunchCode();
      }
      MarketState memory state = _getUpdatedState();
@>    _blockAccount(state, accountAddress); //@audit -- invoke the _blockAccount() to block the sanctioned lender
      _writeState(state);
    }

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol
    function _blockAccount(MarketState memory state, address accountAddress) internal {
      Account memory account = _accounts[accountAddress];
      if (account.approval != AuthRole.Blocked) {
        ...

        if (scaledBalance > 0) {
          account.scaledBalance = 0;
          address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
@>          accountAddress, //@audit -- the incorrect Escrow for the market tokens is created
@>          borrower,
            address(this)
          );
          emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
@>        _accounts[escrow].scaledBalance += scaledBalance; //@audit -- all blocked lender's market tokens are sent to the Escrow
          emit SanctionedAccountAssetsSentToEscrow(
            accountAddress,
            escrow,
            state.normalizeAmount(scaledBalance)
          );
        }
        _accounts[accountAddress] = account;
      }
    }

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatSanctionsSentinel.sol
    function createEscrow(
@>    address borrower, //@audit -- notice the difference between the function params and the passed arguments
@>    address account,
      address asset
    ) public override returns (address escrowContract) {
      ...

@>    tmpEscrowParams = TmpEscrowParams(borrower, account, asset); //@audit -- now, the borrower == address(blocked lender) and account == address(legit borrower)

@>    new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }(); //@audit -- create an incorrect Escrow that the (legit) borrower can execute Escrow::releaseEscrow() to steal the blocked lender's tokens

      ...
    }

Explaining how the executeWithdrawal() creates incorrect Escrows for the market tokens and the asset tokens

When a sanctioned lender (but has not been blocked from the market) executes the WildcatMarketWithdrawals::executeWithdrawal(), the function will invoke the WildcatMarketBase::_blockAccount() to block the lender from the market and then transfer all their market tokens to a created Escrow.

In this step, the _blockAccount() will create an incorrect Escrow for the blocked lender's market tokens, allowing the borrower to steal all locked market tokens. For a detailed explanation, please refer to the Explaining how the nukeFromOrbit() creates an incorrect Escrow for the market tokens section above.

Then, the executeWithdrawal() will execute the WildcatSanctionsSentinel::createEscrow() to create an Escrow for holding the blocked lender's asset tokens (from a pending withdrawal request that has expired). Similar to the _blockAccount(), the executeWithdrawal() will pass the accountAddress and borrower arguments into the createEscrow() alternately with its required parameters.

For this reason, the createEscrow()'s borrower parameter will point to the blocked lender, whereas the account parameter will point to the borrower unintentionally. Hence, the createEscrow() will create the incorrect Escrow contract.

Lastly, all blocked lender's asset tokens (from a pending withdrawal request that has expired) will be transferred to the incorrectly created Escrow, allowing the borrower to execute the WildcatSanctionsEscrow::releaseEscrow() on the Escrow to steal all asset tokens.

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol
    function executeWithdrawal(
      address accountAddress,
      uint32 expiry
    ) external nonReentrant returns (uint256) {
      ...

      if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
@>      _blockAccount(state, accountAddress); //@audit -- invoke the _blockAccount() to block the sanctioned lender
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
@>        accountAddress, //@audit -- the incorrect Escrow for the asset tokens (underlying assets) is created
@>        borrower,
          address(asset)
        );
@>      asset.safeTransfer(escrow, normalizedAmountWithdrawn); //@audit -- all blocked lender's asset tokens (from a pending withdrawal request that has expired) are sent to the Escrow
        emit SanctionedAccountWithdrawalSentToEscrow(
          accountAddress,
          escrow,
          expiry,
          normalizedAmountWithdrawn
        );
      } else {
        ...
      }

      ...
    }

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol
    function _blockAccount(MarketState memory state, address accountAddress) internal {
      Account memory account = _accounts[accountAddress];
      if (account.approval != AuthRole.Blocked) {
        ...

        if (scaledBalance > 0) {
          account.scaledBalance = 0;
          address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
@>          accountAddress, //@audit -- the incorrect Escrow for the market tokens is created
@>          borrower,
            address(this)
          );
          emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
@>        _accounts[escrow].scaledBalance += scaledBalance; //@audit -- all blocked lender's market tokens are sent to the Escrow
          emit SanctionedAccountAssetsSentToEscrow(
            accountAddress,
            escrow,
            state.normalizeAmount(scaledBalance)
          );
        }
        _accounts[accountAddress] = account;
      }
    }

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/WildcatSanctionsSentinel.sol
    function createEscrow(
@>    address borrower, //@audit -- notice the difference between the function params and the passed arguments
@>    address account,
      address asset
    ) public override returns (address escrowContract) {
      ...

@>    tmpEscrowParams = TmpEscrowParams(borrower, account, asset); //@audit -- now, the borrower == address(blocked lender) and account == address(legit borrower)

@>    new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }(); //@audit -- create an incorrect Escrow that the (legit) borrower can execute Escrow::releaseEscrow() to steal the blocked lender's tokens

      ...
    }

Step-by-step exploit scenarios

There are two exploit scenarios based on the sanctioned lender (victim)'s actions prior to the sanction event.

  1. If the sanctioned lender (victim) has no pending withdrawal request:

    1. A borrower (attacker) executes the WildcatMarketConfig::nukeFromOrbit() to block the lender and transfer all lender's market tokens to the incorrectly created Escrow.

    2. The borrower invokes the WildcatSanctionsEscrow::releaseEscrow() on the Escrow to steal the locked market tokens. Because the account parameter will point to the borrower (who is not sanctioned by Chainalysis), the check for the release authorization by the canReleaseEscrow() will be passed (more refs: #1 and #2). The market tokens will be transferred to the borrower.

    3. The borrower executes the WildcatMarketController::authorizeLenders() to authorize themselves as a lender and then calls the WildcatMarketController::updateLenderAuthorization() to apply the lender authorization to the target market.

    4. The borrower triggers the WildcatMarketWithdrawals::queueWithdrawal() to create a pending request for withdrawing (underlying) asset tokens by burning the stolen market tokens for exchange. As a result of Step 3, the borrower now becomes a legitimate lender with the approval status == AuthRole.DepositAndWithdraw. Therefore, the check for the withdrawal authorization by the _getAccountWithRole() will be passed.

    5. Once the pending withdrawal request has expired, the borrower triggers the WildcatMarketWithdrawals::executeWithdrawal() to withdraw the blocked lender's (underlying) asset tokens from the market. The stolen asset tokens will be transferred to the borrower.

  2. If the sanctioned lender (victim) has previously executed some pending withdrawal requests before being sanctioned:

    1. A borrower (attacker) executes the WildcatMarketWithdrawals::executeWithdrawal() to block the lender (if necessary) and transfer both the market tokens and underlying asset tokens (from a pending withdrawal request that has expired) of the blocked lender to the incorrectly created Escrows (There will be 2 Escrows -- one for the market tokens and another for the asset tokens).

    2. The borrower invokes the WildcatSanctionsEscrow::releaseEscrow() on both Escrows to steal the locked market tokens and the underlying asset tokens. Because the account parameter will point to the borrower (who is not sanctioned by Chainalysis), the check for the release authorization by the canReleaseEscrow() will be passed (more refs: #1 and #2). Finally, both the market tokens and asset tokens will be transferred to the borrower. At this step, the borrower has successfully stolen certain asset tokens. To steal the remaining tokens, perform the next step.

    3. The borrower must burn the stolen market tokens for the underlying asset tokens via the process of executing the WildcatMarketWithdrawals::queueWithdrawal() and WildcatMarketWithdrawals::executeWithdrawal() similar to Steps 1.3 - 1.5 above.

Tools Used

Manual Review

Recommended Mitigation Steps

To fix the vulnerability, swap the passing arguments (accountAddress and borrower) in the WildcatMarketBase::_blockAccount() and in the WildcatMarketWithdrawals::executeWithdrawal(), like the snippet below.

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketBase.sol
    function _blockAccount(MarketState memory state, address accountAddress) internal {
      Account memory account = _accounts[accountAddress];
      if (account.approval != AuthRole.Blocked) {
        ...

        if (scaledBalance > 0) {
          account.scaledBalance = 0;
          address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
-           accountAddress,
-           borrower,
+           borrower,
+           accountAddress,
            address(this)
          );
          emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
          _accounts[escrow].scaledBalance += scaledBalance;
          emit SanctionedAccountAssetsSentToEscrow(
            accountAddress,
            escrow,
            state.normalizeAmount(scaledBalance)
          );
        }
        _accounts[accountAddress] = account;
      }
    }

    // FILE: https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarketWithdrawals.sol
    function executeWithdrawal(
      address accountAddress,
      uint32 expiry
    ) external nonReentrant returns (uint256) {
      ...

      if (IWildcatSanctionsSentinel(sentinel).isSanctioned(borrower, accountAddress)) {
        _blockAccount(state, accountAddress);
        address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
-         accountAddress,
-         borrower,
+         borrower,
+         accountAddress,
          address(asset)
        );
        asset.safeTransfer(escrow, normalizedAmountWithdrawn);
        emit SanctionedAccountWithdrawalSentToEscrow(
          accountAddress,
          escrow,
          expiry,
          normalizedAmountWithdrawn
        );
      } else {
        ...
      }

      ...
    }

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #515

c4-judge commented 1 year ago

MarioPoneder marked the issue as satisfactory