c4-submissions commented 10 months ago

Lines of code

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/WildcatMarketWithdrawals.sol#L167-L168

Vulnerability details

Impact

getEscrowAddress() returns the wrong WildcatSanctionsEscrow. Borrower can steal lender's escrowed funds.

Proof of concept

createEscrow() and getEscrowAddress() both take the parameters borrower, account, asset, in that order, as defined in WildcatSanctionsSentinel.sol. However, where createEscrow() is used, in WildcatMarketBase._blockAccount() and in WildcatMarketWithdrawals.executeWithdrawal(), the intended borrower and account have swapped places.

This means that getEscrowAddress(borrower, account, asset) returns a different address. The immediate implication of this is that a user or contract interfacing with getEscrowAddress() will be unable to find the correct WildcatSanctionsEscrow.

Furthermore, the borrower and account will then have swapped places also in the deployed WildcatSanctionsEscrow. This means that canReleaseEscrow() now returns !WildcatSanctionsSentinel(sentinel).isSanctioned(account, borrower); which most likely is true since it was the account that was sanctioned, not the borrower. Then the borrower can releaseEscrow() which will now IERC20(asset).transfer(borrower, amount);. That is, the borrower can immediately transfer the lender's escrowed funds to himself.

createEscrow() will now also set sanctionOverrides[account][escrowContract] = true;, which seems less of an issue.

Recommended mitigation steps

Correct the order of the parameters in WildcatMarketBase._blockAccount() and in WildcatMarketWithdrawals.executeWithdrawal().

Assessed type

Context

c4-pre-sort commented 10 months ago

minhquanym marked the issue as duplicate of #515

c4-judge commented 10 months ago

MarioPoneder changed the severity to 3 (High Risk)