cheatcode - Improper Validation in DelayedOrder's announceStableDeposit can Corrupt State via Calldata Padding

[sherlock-admin]

cheatcode

high

Improper Validation in DelayedOrder's announceStableDeposit can Corrupt State via Calldata Padding

Summary

The announceStableDeposit function in the DelayedOrder smart contract is vulnerable to calldata padding attacks due to improper validation of the length and format of parameters passed to the function.

Vulnerability Detail

The announceStableDeposit function expects calldata containing three uint256 parameters - depositAmount, minAmountOut, and keeperFee. The function does not check that the calldata length or structure matches what is expected. An attacker can craft exploitational calldata with three valid uint256 values for the expected parameters, followed by additional padding bytes. When the contract parses this calldata, it interprets the extra padding data incorrectly, leading to corruption of critical state variables tracking deposits and balances.

PoC Test

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;

import "forge-std/Test.sol";
import "../src/DelayedOrder.sol"; 

contract DelayedOrderTest is Test {
    DelayedOrder delayedOrder;

    function setUp() public {
        // Deploy the DelayedOrder contract
        delayedOrder = new DelayedOrder();
        // Initialize and set up your contract as needed
    }

    function testAnnounceStableDepositWithExtraCalldata() public {
        // Example calldata for announceStableDeposit
        uint256 depositAmount = 1e18;
        uint256 minAmountOut = 1e18;
        uint256 keeperFee = 1e16;

        // Prepare calldata with extra padding
        bytes memory paddedCalldata = abi.encodeWithSelector(
            delayedOrder.announceStableDeposit.selector,
            depositAmount,
            minAmountOut,
            keeperFee,
            uint256(123) // Extra padding data
        );

        // Call the function with padded calldata
        (bool success, ) = address(delayedOrder).call(paddedCalldata);

        // Check if the transaction failed (expected behavior after the fix)
        assertFalse(success);
    }
}

Run the test using Foundry:

forge test --match-contract DelayedOrderTest --match-test testAnnounceStableDepositWithExtraCalldata

Impact

1. Allows attacker to deposit funds without transferring sufficient collateral
2. Distorts accounting flows in the protocol due to state corruption
3. Financial losses for legitimate users due to inflated balances

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L67

Tool used

Manual Review

Recommendation

Add calldata length and format checks

[sherlock-admin]

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

takarez commented:

invalid

[nevillehuang]

Invalid I believe the PoC is not proving the issue. If anything it seems to be correct that the announceStableDeposit() function reverts accordingly represented by assertFalse(success)