The Wildcat Protocol - 0xDING99YA's results

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L87-L120

Vulnerability details

Impact

createEscrow() will return the escrow contract address if it was already deployed, or deploy a new one if not. However, attacker can make the createEscrow() always return the escrow address without code deployed, sanctioned funds will be transferred to that address and since there is no code there, there is no way to recover these funds.

Proof of Concept

createEscrow() will first get the escrow contract address and use code hash to determine if it is a contract.

escrowContract = getEscrowAddress(borrower, account, asset);

if (escrowContract.codehash != bytes32(0)) return escrowContract;

If escrowContract.codehash != bytes32(0), it will consider the escrowContract was already deployed and return that address. If escrowContract does not exist or it's empty, 0x0 code hash will be returned. See here: https://eips.ethereum.org/EIPS/eip-1052

However, the issue here is that an attacker can send 1 Wei to the not yet created escrowContract, that will make the escrowContract not defined as empty anymore since it has balance now, and the returned code hash will be c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470, which is not 0. The definition of empty address can be found here: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-161.md

As a result, since the escrowContratc has 1 Wei balance and will return a non-zero code hash, createEscrow() will directly return that address and never create the real escrow. Funds will still be transferred to that escrowContract when an account is sanctioned, but later when the account is not sanctioned and want to retrieve those funds it will not work since the escrow is basically an EOA.

Tools Used

Manual Review

Recommended Mitigation Steps

Use external code size instead to determine if an address is contract or not

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L157-L187 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L164-L170 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/WildcatSanctionsEscrow.sol#L16-L19

Vulnerability details

Impact

When creating an escrow for an account, the parameters used should be in "borrower, account, asset" order, however, in _blockAccount() and executeWithdrawals(), these two functions use "account, borrower, asset" order when constructing the escrow. The escrow contract created in this way will mark the sanctioned account as the borrower, and the sanctioned account can simply update the sanctionOverride mapping to get their funds back when it is still under sanctioned.

Proof of Concept

From WildcatSanctionsSentinel.sol and WildcatSanctionsEscrow.sol we can see that the correct parameters when constructing the escrow should be in order "borrower, account, asset".

tmpEscrowParams = TmpEscrowParams(borrower, account, asset);

new WildcatSanctionsEscrow{ salt: keccak256(abi.encode(borrower, account, asset)) }();

constructor() {
  sentinel = msg.sender;
  (borrower, account, asset) = WildcatSanctionsSentinel(sentinel).tmpEscrowParams();
}

However, in _blockAccount() and executeWithdrawl() the order is not correct.

address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
    accountAddress,
    borrower,
    address(asset)
  );

if (scaledBalance > 0) {
    account.scaledBalance = 0;
    address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
      accountAddress,
      borrower,
      address(this)
    );

We can see it uses "account, borrower, asset" instead of "borrower, account, asset". So the escrow created will mark the real sanctioned account as the borrower, mark the real borrower as the sanctioned address and then transfer the funds. Therefore the real sanctioned address can simply call overrideSanction() and then transfer the funds back (Actually they don't even need to do that because isSanctioned() will return false for the real borrower).

Tools Used

Manual Review

Recommended Mitigation Steps

Just make sure when constructing the escrow the parameters used are correct

Assessed type

Other