Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: high
Invalid

Storage Collision in load Function

kineticsofweb3

Summary

The load(uint128 vaultId) function dynamically computes a storage slot using keccak256. However, Solidity does not provide direct support for dynamically assigned storage slots within libraries, which can lead to unintended storage collisions. This vulnerability can result in overwriting or corrupting critical storage variables in the contract.

Vulnerability Details

  • Function Affected: load(uint128 vaultId)

  • Issue: The function calculates the storage slot using keccak256 dynamically. However, Solidity does not allow library functions to define arbitrary storage slots safely. This can lead to storage overlaps when the contract deploys or upgrades, causing unintended data corruption.

  • Cause: Solidity does not provide a built-in way to manage dynamic storage slots within libraries, making this approach unsafe.

  • Severity: High

  • Likelihood: Medium

  • Exploitability: Moderate

Root Cause

  • The load function attempts to compute storage locations dynamically using keccak256, which is problematic in Solidity libraries.

  • Since Solidity does not have explicit support for dynamically assigned storage slots in libraries, unintended storage collisions can occur if the contract writes data to an already occupied storage slot.

  • If multiple contracts use this library without proper safeguards, critical storage data can be overwritten.

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/35deb3e92b2a32cd304bf61d27e6071ef36e446d/src/market-making/leaves/Vault.sol#L151

Impact

  • Potential data corruption: Existing storage variables may be unintentionally modified, leading to unpredictable contract behavior.

  • Loss of funds: If storage corruption affects user balances or vaults, assets may be misallocated or lost.

  • Increased security risk: Attackers may be able to exploit storage collisions to manipulate contract data maliciously.

Tools Used

  • Hardhat: For testing and simulating the vulnerability.

  • Slither: For static analysis and vulnerability detection.

  • Foundry: For fuzz testing and differential analysis.

Proof of Concept (PoC)

To demonstrate the vulnerability, i create a Hardhat test to simulate storage corruption due to storage slot collision.

Hardhat Test Code

const { expect } = require("chai");
​
describe("Storage Collision in load Function", function () {
let StorageCollision, storageCollision, owner;
​
before(async function () {
[owner] = await ethers.getSigners();
​
// Deploy contract
const StorageCollision = await ethers.getContractFactory("StorageCollision");
storageCollision = await StorageCollision.deploy();
await storageCollision.deployed();
});
​
it("should overwrite data due to storage slot collision", async function () {
// Store data in the first vault
await storageCollision.store(1, "First Data");
​
// Store data in another vault (potentially colliding)
await storageCollision.store(2, "Second Data");
​
// Load first vault and check if data is intact
const firstData = await storageCollision.load(1);
console.log("First Data:", firstData);
​
// Load second vault and check if data is intact
const secondData = await storageCollision.load(2);
console.log("Second Data:", secondData);
​
// Check if the data got overwritten
expect(firstData).to.not.equal(secondData);
});
});

Mitigation Strategy

To prevent storage collisions, avoid dynamically computing storage slots and instead use a mapping in a contract for storing vault data.

Recommended Fix:

Replace the current load function with a mapping-based approach:

contract SafeVault {
mapping(uint128 => bytes32) private vaults;
​
function store(uint128 vaultId, bytes32 data) external {
vaults[vaultId] = data;
}
​
function load(uint128 vaultId) external view returns (bytes32) {
return vaults[vaultId];
}
}

Benefits of the Fix

  • No storage collision risk: The mapping ensures each vaultId has a unique storage location.

  • Solidity best practices: Aligns with Solidity's native storage management instead of relying on dynamic slot computation.

  • Improved security: Prevents unintended overwrites of contract state.

Refactor the contract to use mapping(uint128 => bytes32) instead of dynamically computed storage slots.

Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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