`withdrawFor` can be used to lock some users asset

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/abstract/RewardControl.sol#L96-L98

Vulnerability details

Impact

Rewards could be lost/stuck for some users.

Proof of Concept

See RewardControl.sol#L96-L98

    function withdrawFor(address from, uint256 amount) external {
        _withdraw(from, from, amount);
    }

This function is used to withdraw rewards on behalf of an address, issue here however is the fact that the function is publicly available to anyone, now note that from here, i.e the signature verification logic we can see that Phi expects smart contracts/wallets to integrate with it's protocol which is why SignatureCheckerLib.isValidSignatureNowCalldata() is used to verify the signatures, which also the ERC1271 to check for contract, see the implementation here:

    function isValidSignatureNowCalldata(address signer, bytes32 hash, bytes calldata signature)
        internal
        view
        returns (bool isValid)
    {
        /// @solidity memory-safe-assembly
        assembly {
            // Clean the upper 96 bits of `signer` in case they are dirty.
            for { signer := shr(96, shl(96, signer)) } signer {} {
                let m := mload(0x40)
                mstore(0x00, hash)
                if eq(signature.length, 64) {
                    let vs := calldataload(add(signature.offset, 0x20))
                    mstore(0x20, add(shr(255, vs), 27)) // `v`.
                    mstore(0x40, calldataload(signature.offset)) // `r`.
                    mstore(0x60, shr(1, shl(1, vs))) // `s`.
                    let t :=
                        staticcall(
                            gas(), // Amount of gas left for the transaction.
                            1, // Address of `ecrecover`.
                            0x00, // Start of input.
                            0x80, // Size of input.
                            0x01, // Start of output.
                            0x20 // Size of output.
                        )
                    // `returndatasize()` will be `0x20` upon success, and `0x00` otherwise.
                    if iszero(or(iszero(returndatasize()), xor(signer, mload(t)))) {
                        isValid := 1
                        mstore(0x60, 0) // Restore the zero slot.
                        mstore(0x40, m) // Restore the free memory pointer.
                        break
                    }
                }
                if eq(signature.length, 65) {
                    mstore(0x20, byte(0, calldataload(add(signature.offset, 0x40)))) // `v`.
                    calldatacopy(0x40, signature.offset, 0x40) // `r`, `s`.
                    let t :=
                        staticcall(
                            gas(), // Amount of gas left for the transaction.
                            1, // Address of `ecrecover`.
                            0x00, // Start of input.
                            0x80, // Size of input.
                            0x01, // Start of output.
                            0x20 // Size of output.
                        )
                    // `returndatasize()` will be `0x20` upon success, and `0x00` otherwise.
                    if iszero(or(iszero(returndatasize()), xor(signer, mload(t)))) {
                        isValid := 1
                        mstore(0x60, 0) // Restore the zero slot.
                        mstore(0x40, m) // Restore the free memory pointer.
                        break
                    }
                }
                mstore(0x60, 0) // Restore the zero slot.
                mstore(0x40, m) // Restore the free memory pointer.

                let f := shl(224, 0x1626ba7e)
                mstore(m, f) // `bytes4(keccak256("isValidSignature(bytes32,bytes)"))`.
                mstore(add(m, 0x04), hash)
                let d := add(m, 0x24)
                mstore(d, 0x40) // The offset of the `signature` in the calldata.
                mstore(add(m, 0x44), signature.length)
                // Copy the `signature` over.
                calldatacopy(add(m, 0x64), signature.offset, signature.length)
                // forgefmt: disable-next-item
                isValid := and(
                    // Whether the returndata is the magic value `0x1626ba7e` (left-aligned).
                    eq(mload(d), f),
                    // Whether the staticcall does not revert.
                    // This must be placed at the end of the `and` clause,
                    // as the arguments are evaluated from right to left.
                    staticcall(
                        gas(), // Remaining gas.
                        signer, // The `signer` address.
                        m, // Offset of calldata in memory.
                        add(signature.length, 0x64), // Length of calldata in memory.
                        d, // Offset of returndata.
                        0x20 // Length of returndata to write.
                    )
                )
                break
            }
        }
    }

Coupling these windows however, this then allows for a simple griefing window that locks a user's asset.

• Consider a smart contract integrates with Phi freqquently.
• Due to the native logic (code), it can't handle the rewards primarily and normally have to query the normal withdraw(address to, uint256 amount) in order to send the rewards to the destined to.

NB: The above can be due to any reason, be it from as simple as this contract just lacks this functionality since they can just query the normal withdraw(), to even more complex cases where we have the contract receiving the rewards to have some complex logic on deciding who among its users would be the recipient of the reward for that duration, etc. Also we'd assume this is a viable scenario since no restriction was place on who/what type of contract can integrate.

• A griefer can now at no cost whatsoever, just either frontrun the contract's attempt at withdrawing rewards to a different to address by calling withdrawFrom() which then locks the funds in the contract and this also doesn't neceassirly need to be a frontrun since the griefer can just query the function whenever some rewardds are accrued.

Recommended Mitigation Steps

Consider not allowing a public withdrawal logic availabkle to everyone, or make every integrator provide their prefferred primary reward address and then during withdrawals if withdrawFor() is used get the primary reward address from the mapping and send the reward to it. Or do not allow contracts/wallets to integrate (unadvisable).

Assessed type

Other