032.md

Magic Rainbow Locust

High

An Attacker Can Overwrite Storage Variables, Leading to Security Bypass and Potential Loss of User Funds

Summary

An incorrect inheritance order in SmartCommitmentForwarder.sol will cause a critical vulnerability for users, as an attacker will be able to overwrite crucial storage variables due to storage slot collisions. This allows the attacker to bypass security mechanisms, such as the nonReentrant modifier, and manipulate the contract state in unintended ways, potentially leading to fund loss or other severe consequences.

Root Cause

In [SmartCommitmentForwarder.sol](https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/SmartCommitmentForwarder.sol#L51-L59), the incorrect order of inheritance leads to storage variables being overwritten due to overlapping storage slots. The contract inherits from ReentrancyGuardUpgradeable, OwnableUpgradeable, and PausableUpgradeable after introducing new storage variables, which causes the storage layout to be misaligned.

contract SmartCommitmentForwarder is  
    ExtensionsContextUpgradeable, // Should always be first for upgradeability  
    TellerV2MarketForwarder_G3,  
    PausableUpgradeable,          // Adds storage variables but is inherited after other contracts  
    ReentrancyGuardUpgradeable,   // Adds storage variables, potentially overwriting existing ones  
    OwnableUpgradeable,           // Adds storage variables but is inherited after own variables  
    OracleProtectionManager,      // Uses deterministic storage slots  
    ISmartCommitmentForwarder,  
    IPausableTimestamp  
{  
    // Contract body  
    uint256 public liquidationProtocolFeePercent;  
    uint256 internal lastUnpausedAt;  
  
    // ... rest of the contract code ...  
}  

The storage variables from the base contracts (ReentrancyGuardUpgradeable, OwnableUpgradeable, and PausableUpgradeable) are inherited after the contract's own storage variables are declared, leading to storage slot collisions and overwriting essential variables.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. The attacker calls a function protected by the nonReentrant modifier:

   • For example, the acceptSmartCommitmentWithRecipient function, which is marked as nonReentrant.

2. Due to storage collisions, the nonReentrant modifier does not function correctly:

   • The _status variable from ReentrancyGuardUpgradeable, used to track reentrancy status, is overwritten or contains an incorrect value.

3. The attacker initiates a reentrant call into the contract:

   • Because the nonReentrant protection is compromised, the attacker can recursively call the same function or another vulnerable function before the first call completes.

4. The attacker manipulates the contract's state or drains funds:

   • The reentrancy allows the attacker to bypass logical checks, manipulate state variables, or withdraw funds multiple times.

Impact

The users of the contract suffer a critical loss of funds as the contract's state is compromised, and security mechanisms are bypassed. This could result in theft of funds from the contract, disruption of expected behaviors, and loss of trust in the platform.

• Potential loss of all funds held by the contract.
• Bypassing of security mechanisms like the nonReentrant modifier.
• Ability for an attacker to manipulate essential state variables.

PoC

While a full PoC requires deployment and testing tools, the attack can be conceptually demonstrated:

1. Setup:

   • The SmartCommitmentForwarder contract is deployed with the incorrect inheritance order.
   • The attacker is aware of the storage collision vulnerability.

2. Attack Execution:

   • The attacker calls the acceptSmartCommitmentWithRecipient function, which is marked as nonReentrant.

   • Due to the storage collision, the _status variable from ReentrancyGuardUpgradeable is overwritten and does not prevent reentrancy.

   • The attacker crafts the transaction to re-enter the acceptSmartCommitmentWithRecipient function before the first call completes.

3. Result:

   • The attacker is able to bypass the nonReentrant modifier.

   • The attacker can manipulate contract state, such as increasing balances or withdrawing funds multiple times.

Mitigation

No response