Omission of empty data check in upgradeAndCall() may result in stuck ether.

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/proxy/AMProxyAdmin.sol#L40-L46

Vulnerability details

Impact

According to OpenZeppelin Docs, the upgradeToAndCall() function:

Performs implementation upgrade with additional setup call if data is nonempty. This function is payable only if the setup call is performed, otherwise msg.value is rejected to avoid stuck value in the contract.

Also in AMProxyAdmin::upgradeAndCall() function, NatSpec states that:

If `data` is empty, `msg.value` must be zero.

However, the function does not explicitly check for this. If data is empty, indicating no setup code is needed, then sending Ether (msg.value) with the transaction could be unnecessary may end up being locked.

Proof of Concept

• https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/proxy/AMProxyAdmin.sol#L40-L46

function upgradeAndCall(
        IAMTransparentUpgradeableProxy proxy,
        address implementation,
        bytes memory data
    ) public payable virtual restricted {

        //@audit Missing check for empty data

        proxy.upgradeToAndCall{value: msg.value}(implementation, data);
    }

The upgradeAndCall() function is designed to upgrade a proxy to a new implementation and then execute some additional code (data).

• Unintended Ether Transfer: Without the check, Ether may be sent with a transaction that should not require Ether, leading to unnecessary Ether accumulation in the contract. This could result in funds being locked in the contract, making it impossible for those funds to be withdrawn or used, potentially causing loss of funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Include checks in your contract to reject Ether if it is not expected. For example, use a require statement to ensure that msg.value is zero when data is empty.

Assessed type

Invalid Validation

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid