sherlock-audit / 2024-05-elfi-protocol-judging

0 stars 0 forks source link

Zero Storage Slot Usage for `AppStorage` leads to potentially vulnerable storage mechanism #294

Closed sherlock-admin3 closed 2 weeks ago

sherlock-admin3 commented 2 weeks ago

Zero Storage Slot Usage for AppStorage leads to potentially vulnerable storage mechanism

Low/Info issue submitted by engineer

Summary

Use of a 0 storage slot mechanism may lead to potential issues.

Vulnerability Detail

The load function in the AppStorage library uses an assembly block to set the storage slot to 0. This approach assumes that the Props struct will always be the first state variable declared in any contract that uses this library. If this assumption is violated, especially in the context of contract inheritance, it can lead to storage collisions and unexpected behavior.

Impact

Misalignment of storage slots can lead to data corruption, unintended overwrites, and significant security vulnerabilities, potentially allowing attackers to exploit these misalignments for malicious purposes.

Code Snippet

The load function implementation as seen here :

    function load() public pure returns (Props storage self) {
        assembly {
            self.slot := 0
        }
    }

Tool used

Manual Review

Recommendation

Instead of ensuring Props is always the first state variable through documentation and code review, modify the load function to accept the storage slot as a parameter, allowing for greater flexibility and reducing the risk of storage misalignment.

    function load(bytes32 slot) public pure returns (Props storage self) {
        assembly {
            self.slot := slot
        }
    }
sherlock-admin2 commented 1 week ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/65