PatrickAlphaC
commented
1 year ago This is a good pattern suggestion, however, we need proof that there are funds at risk. In it's current state, this finding talks about hypothetical issues that can occur from not following best practices.
For this to be accepted, we need proof of a vulnerability.
Malicious lender might be able to take more loan tokens than deposited to lender.sol contract due to reentrancy
Severity
High Risk
Relevant GitHub Links
https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Lender.sol#L159
https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Lender.sol#L175
Summary
CEI(check,effect and interaction) pattern is not followed due which there can be reentrancy attacks possible.
Vulnerability Details
If the pool lender updates the pool using setPool function and then it decreases it's poolBalance then the lender.sol contract transfers the extra loan tokens in the contract to the pool lender but here comes the attack if the pool lender is a malicious contract then it can call the set function again from the pool lender contract and as the updates to the pool are made at the end so if the set pool is called again by the malicious pool lender contract it would appear as if the function is called first time and again the lender.sol contract transfers the loan tokens to the pool lender.
Impact
Malicious pool lender can drain the loan tokens from the lender.sol contract
Tools Used
Manual Review
Recommendations
Implement CEI pattern correctly firstly update the changes then make external calls