drextron - Core functions of Lending logic are susceptible to reentrancy attacks that could severely compromise system account balances and other important accounting data in potentially calamitous ways

[sherlock-admin2]

drextron

high

Core functions of Lending logic are susceptible to reentrancy attacks that could severely compromise system account balances and other important accounting data in potentially calamitous ways

---

name: Audit item #1 about: "Memory update -> then business logic -> then save to storage" pattern + lack of reentrancy guards leave Aloe open to attackers manipulating protocol-crucial accounting data title: "Core functions of Lending logic are susceptible to reentrancy attacks that could severely compromise system account balances and other important accounting data in potentially calamitous ways" labels: "High severity"

Summary

The code pattern, which is widely used in the Aloe codebase, of "write to memory, execute some business logic, and then save to storage", opens up several reentrancy vulnerabilities in the codebase. Some significant ones are found in the Lender.sol contract.

Vulnerability Detail

In Lender.sol, the private function _save updates state variables, according to values changed in memory and passed to the function via the params. The way in which this pattern is used involves typically reading some memory/storage variables, making in-memory changes, and then eventually calling _save to save these updates to state. These methods that call _save do not particularly do anything stop the following cases, that might result in multiple unintended changes to memory before a single state update is made, which would of course result in unintended/incorrect/malicious/inaccurate state updates:

• a malicious actor calling the same public/external function which internally calls _save more than once in quick succession, to force more than one memory update to happen quickly enough so that corresponding state updates are not made in correct atomic, sequential order
• the same case as above, except involving multiple honest actors making honest calls to functions calling _save involving some logic that makes updates to the same memory variables before corresponding state updates are made (i.e. two separate external methods that both updates the balance for an address, called by two different honest actors but for the same beneficiary address)

Impact

The potential impacts can be very severe, from resulting in inaccurate totalSupply tallies, to double deposits with the same funds, to a number of other possibilities that would results in the Lending part of the Aloe protocol having inaccurate accounting. Here are some cases in Lender.sol (each of these functions employ some form of this reentrant-susceptible update pattern + usage of _save):

function deposit(uint256 amount, address beneficiary, uint32 courierId) public returns (uint256 shares) [https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L145]

• new shares are minted and added to totalSupply and lastBalance -> first in memory and then in storage
• if an actor calls this method multiple times in quick succession, these values changes can possibly be updated in memory more than once before for the same state change, before the corresponding state changes are done in the right order correctly and as intended

function redeem(uint256 shares, address recipient, address owner) public returns (uint256 amount) [https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L198]

• existing shares are burned and added to totalSupply and lastBalance -> first in memory and then in storage
• if an actor calls this method multiple times in quick succession, these values changes can possibly be updated in memory more than once before for the same state change, before the corresponding state changes are done in the right order correctly and as intended

function borrow(uint256 amount, address recipient) external returns (uint256 units) [https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L235]

• values for borrowBase and lastBalance are updated -> first in memory and then in storage
• if an actor calls this method multiple times in quick succession, these values changes can possibly be updated in memory more than once before for the same state change, before the corresponding state changes are done in the right order correctly and as intended

function repay(uint256 amount, address beneficiary) external returns (uint256 units) [https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Lender.sol#L281]

• value for lastBalance is updated -> first in memory and then in storage
• if an actor calls this method multiple times in quick succession, these values changes can possibly be updated in memory more than once before for the same state change, before the corresponding state changes are done in the right order correctly and as intended

In each of these cases, memory updates can be done more than once before state updates each time via malicious or onest actors re-enter (intentionally or unintentionally, respectively) to manipulate the correctness of values in Aloe's accounting state.

Being that these reentrancy cases outlined in detail here concern core lending functionalities dealing with user and system balances (deposit, redeem, borrow, repay), a certain and/or definite loss of funds can easily be inflicted by an attacker

Code Snippet

Tool used

Solidity

Recommendation

Any functions that uses _save or similar state changing functions when using the "write to memory, execute some business logic, and then save to storage" pattern should use a reentrancy guard, like OpenZeppelin's nonReentrant() modifier [https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard-nonReentrant--] or update the memory-to-state coding pattern discussed earlier in this issue.

[sherlock-admin2]

4 comment(s) were left on this issue during the judging contest.

panprog commented:

invalid, because it doesn't show any valid path to actually achieve reentrancy: all paths should be unable to reenter due to code logic. All the report talks about it potential problems, but there are no real problems with this.

tsvetanovv commented:

I don't think reentrancy can happen

MohammedRizwan commented:

invalid issue

chrisling commented:

reentrancy guard is implemented via lastAccrualTime, which is always set to 0 after reentrancy check. See _load -> _previewInterest, and unlocked with _save. There is also no proof of concept to prove that reentrancy attack is possible.