GimelSec - User/Gov's quoteToken allowance which is approved for `depositInsurance()` will be maliciously used on `rebalance()`

[github-actions[bot]]

GimelSec

high

User/Gov's quoteToken allowance which is approved for depositInsurance() will be maliciously used on rebalance()

Summary

User/Gov's quoteToken allowance which is approved for depositInsurance() will be maliciously used on rebalance().

Vulnerability Detail

It's a requirement to approve insuranceToken (the same as quoteToken) first before calling depositInsurance().

    function depositInsurance(uint256 amount, address from)
        external
        nonReentrant
        onlyOwner
    {
        if (amount == 0) {
            revert ZeroAmount();
        }
        uint256 allowance = IERC20(insuranceToken()).allowance(
            from,
            address(this)
        );
        if (allowance < amount) {
            revert NotApproved(allowance, amount);
        }

But if the allowance is set, anyone can use this allowance to call rebalance() before the owner calls depositInsurance(). Because rebalance() also requires someone to approve quoteToken first.

If Alice has approved quoteToken for depositInsurance() but hasn't called depositInsurance() yet, an attacker Bob can call rebalance() and set the account parameter to Alice’s account. Alice will spend the allowance and pay for rebalance if shortFall > 0, but this allowance is for depositInsurance().

Impact

It's a common griefing scenario for smart contract attacks that the owner will be unable to call depositInsurance() due to lack of allowance.

Furthermore, it's more serious if the owner is DAO and it needs to wait for DAO members to vote on any executions. The attacker will have more time to call rebalance() before the owner calls depositInsurance(), without the need to frontrun the owner's depositInsurance() transaction.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L179-L193

Tool used

Manual Review

Recommendation

Use a specific account for rebalance() rather than using an account parameter. Or use the modifier onlyOwner on rebalance().

Duplicate of #192