ak1 - Market.sol : `claimReward` need to have reentrancy protection, since the state update is done after making the external call.

[sherlock-admin]

ak1

high

Market.sol : claimReward need to have reentrancy protection, since the state update is done after making the external call.

Summary

Market.sol has claimReward external function. Any valid user can claim any available reward which is accrued. But it updates the state after making the transfer. This is dangerous since it opens for reentrancy attack.

Vulnerability Detail

Lets look at the claimReward function,

    function claimReward() external {
        Local memory newLocal = _locals[msg.sender].read();

        reward.push(msg.sender, UFixed18Lib.from(newLocal.reward)); ------------->>> transfer the reward to msg.sender
        emit RewardClaimed(msg.sender, newLocal.reward);

        newLocal.reward = UFixed6Lib.ZERO; ------------------------->>> reset to zero after the push call.
        _locals[msg.sender].store(newLocal);
    }

Impact

The call re-entered before updating the state till the funds are drained out.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L154C28-L162

Tool used

Manual Review

Recommendation

Use the reentrancy protection for ClaimReward function.

[sherlock-admin]

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

141345 commented:

d

panprog commented:

invalid because owner is trusted

[aktech297]

Escalate

This function claimReward is called by the actual user who have rewards accumulated during market operation. So, there is no meaning for the comment invalid because owner is trusted.

Lets see the contract flow to know why the actual user can be the caller here.

When we look at the MultiInvoker contract's invoke function, different type of perennial actions is done by the user by sending the payment (check this function is payable).

    function invoke(Invocation[] calldata invocations) external payable {
        for(uint i = 0; i < invocations.length; ++i) {
            Invocation memory invocation = invocations[i];

            if (invocation.action == PerennialAction.UPDATE_POSITION) {
                (
                    IMarket market,
                    UFixed6 newMaker,
                    UFixed6 newLong,
                    UFixed6 newShort,
                    Fixed6 collateral,
                    bool wrap
                ) = abi.decode(invocation.args, (IMarket, UFixed6, UFixed6, UFixed6, Fixed6, bool));

                _update(market, newMaker, newLong, newShort, collateral, wrap); ------------>> follow this function
            } else if (invocation.action == PerennialAction.UPDATE_VAULT) {
......

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L164-L185

    /// @notice Updates market on behalf of msg.sender
    /// @param market Address of market up update
    /// @param newMaker New maker position for msg.sender in `market`
    /// @param newLong New long position for msg.sender in `market`
    /// @param newShort New short position for msg.sender in `market`
    /// @param collateral Net change in collateral for msg.sender in `market`
    function _update(
        IMarket market,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool wrap
    ) internal {
        // collateral is transferred from this address to the market, transfer from msg.sender to here
        if (collateral.sign() == 1) _deposit(collateral.abs(), wrap);

        market.update(msg.sender, newMaker, newLong, newShort, collateral, false); ----->>> called the Market's update where the msg.sender is the user.

        // collateral is transferred from the market to this address, transfer to msg.sender from here
        if (collateral.sign() == -1) _withdraw(msg.sender, collateral.abs(), wrap);
    }
.....

Now, let look at the Market's update function,

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L69-L88

    function update(
        address account,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool protect
    ) external nonReentrant whenNotPaused {
        Context memory context = _loadContext(account);
        _settle(context, account);
        _update(context, account, newMaker, newLong, newShort, collateral, protect);
        _saveContext(context, account); ----------------------------------------------------->> follow this function now.
    }

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L319-L322 For each account's the local context is updated in the _locals[account].store(context.local);

mapping(address => LocalStorage) private _locals;

    function _saveContext(Context memory context, address account) private {
        _global.store(context.global);
        _locals[account].store(context.local); -------------------------------------->> for caller, the local context is updated.
    }

Now, lets look at the claimReward function, https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L154-L162

    function claimReward() external {
        Local memory newLocal = _locals[msg.sender].read(); ---------->> for each caller, local context is read.

        reward.push(msg.sender, UFixed18Lib.from(newLocal.reward)); ---------->> reward is send to the msg.sender
        emit RewardClaimed(msg.sender, newLocal.reward);

        newLocal.reward = UFixed6Lib.ZERO; -------------->> reward is set to zero.
        _locals[msg.sender].store(newLocal); ------------>> state is update.
    }

It is clear, that the user is the caller and they can call this function to claim the reward.

since the claimReward function update the state after making the transfer, this is not safe from the reentrancy attack.

This leas to loss funds. So the impact could be High.

I would request to consider this as High.

Thanks!

[sherlock-admin2]

Escalate

This function claimReward is called by the actual user who have rewards accumulated during market operation. So, there is no meaning for the comment invalid because owner is trusted.

Lets see the contract flow to know why the actual user can be the caller here.

When we look at the MultiInvoker contract's invoke function, different type of perennial actions is done by the user by sending the payment (check this function is payable).

    function invoke(Invocation[] calldata invocations) external payable {
        for(uint i = 0; i < invocations.length; ++i) {
            Invocation memory invocation = invocations[i];

            if (invocation.action == PerennialAction.UPDATE_POSITION) {
                (
                    IMarket market,
                    UFixed6 newMaker,
                    UFixed6 newLong,
                    UFixed6 newShort,
                    Fixed6 collateral,
                    bool wrap
                ) = abi.decode(invocation.args, (IMarket, UFixed6, UFixed6, UFixed6, Fixed6, bool));

                _update(market, newMaker, newLong, newShort, collateral, wrap); ------------>> follow this function
            } else if (invocation.action == PerennialAction.UPDATE_VAULT) {
......

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L164-L185

    /// @notice Updates market on behalf of msg.sender
    /// @param market Address of market up update
    /// @param newMaker New maker position for msg.sender in `market`
    /// @param newLong New long position for msg.sender in `market`
    /// @param newShort New short position for msg.sender in `market`
    /// @param collateral Net change in collateral for msg.sender in `market`
    function _update(
        IMarket market,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool wrap
    ) internal {
        // collateral is transferred from this address to the market, transfer from msg.sender to here
        if (collateral.sign() == 1) _deposit(collateral.abs(), wrap);

        market.update(msg.sender, newMaker, newLong, newShort, collateral, false); ----->>> called the Market's update where the msg.sender is the user.

        // collateral is transferred from the market to this address, transfer to msg.sender from here
        if (collateral.sign() == -1) _withdraw(msg.sender, collateral.abs(), wrap);
    }
.....

Now, let look at the Market's update function,

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L69-L88

    function update(
        address account,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool protect
    ) external nonReentrant whenNotPaused {
        Context memory context = _loadContext(account);
        _settle(context, account);
        _update(context, account, newMaker, newLong, newShort, collateral, protect);
        _saveContext(context, account); ----------------------------------------------------->> follow this function now.
    }

https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L319-L322 For each account's the local context is updated in the _locals[account].store(context.local);

mapping(address => LocalStorage) private _locals;

    function _saveContext(Context memory context, address account) private {
        _global.store(context.global);
        _locals[account].store(context.local); -------------------------------------->> for caller, the local context is updated.
    }

Now, lets look at the claimReward function, https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial/contracts/Market.sol#L154-L162

    function claimReward() external {
        Local memory newLocal = _locals[msg.sender].read(); ---------->> for each caller, local context is read.

        reward.push(msg.sender, UFixed18Lib.from(newLocal.reward)); ---------->> reward is send to the msg.sender
        emit RewardClaimed(msg.sender, newLocal.reward);

        newLocal.reward = UFixed6Lib.ZERO; -------------->> reward is set to zero.
        _locals[msg.sender].store(newLocal); ------------>> state is update.
    }

It is clear, that the user is the caller and they can call this function to claim the reward.

since the claimReward function update the state after making the transfer, this is not safe from the reentrancy attack.

This leas to loss funds. So the impact could be High.

I would request to consider this as High.

Thanks!

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[panprog]

Escalate

You're fully correct that generally speaking, this is vulnerable to reentrancy and reentrancy path is very easy and clear, allowing any user to steal all rewards the smart contract has. However, the reward token is set by admin who is trusted. He is supposed to only set trusted tokens (which can't be used to re-enter). If admin sets a token which allows re-entrancy, this is an admin mistake, which is invalid by Sherlock rules. So this should be low.

[sherlock-admin2]

Escalate

You're fully correct that generally speaking, this is vulnerable to reentrancy and reentrancy path is very easy and clear, allowing any user to steal all rewards the smart contract has. However, the reward token is set by admin who is trusted. He is supposed to only set trusted tokens (which can't be used to re-enter). If admin sets a token which allows re-entrancy, this is an admin mistake, which is invalid by Sherlock rules. So this should be low.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[aktech297]

I don't see any protection other than re-entrant check in the token world. Any token that is set can be re-entered when the msg.sender is another contract.

There are no reference about the type of tokens that can be set by the admin. Its really vague to make such assumption that admin has configured everything and it can not be deemed as issue. if that is the case, entire contracts are developed by admin so the issues are not actual issues.

For above reasons, I do stand by the escalation.

[141345]

Reward token is not arbitrary trading pair token, when admin set the reward token, the likelihood that it has transfer hook is quite low.

From best practice aspect, CEI pattern is recommended.

The severity of low seems more appropriate.

[aktech297]

Adding function call flow for reward.push(msg.sender, UFixed18Lib.from(newLocal.reward));

    /**
     * @notice Transfers `amount` tokens from the caller to the `recipient`
     * @param self Token to transfer
     * @param recipient Address to transfer tokens to
     * @param amount Amount of tokens to transfer
     */
    function push(Token18 self, address recipient, UFixed18 amount) internal {
        IERC20(Token18.unwrap(self)).safeTransfer(recipient, UFixed18.unwrap(amount)); ---------------->> OZ safeERC20.sol
    }

Lets look the OZ SafeERC20 flow

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/812404cee89cd91d46f6234d03451b07eebf2e35/contracts/token/ERC20/utils/SafeERC20.sol#L36-L38

    function safeTransfer(IERC20 token, address to, uint256 value) internal {
        _callOptionalReturn(token, abi.encodeCall(token.transfer, (to, value)));
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/812404cee89cd91d46f6234d03451b07eebf2e35/contracts/token/ERC20/utils/SafeERC20.sol#L113-L122

    function _callOptionalReturn(IERC20 token, bytes memory data) private {
        // We need to perform a low level call here, to bypass Solidity's return data size checking mechanism, since
        // we're implementing it ourselves. We use {Address-functionCall} to perform this call, which verifies that
        // the target address contains contract code and also asserts for success in the low-level call.

        bytes memory returndata = address(token).functionCall(data);
        if (returndata.length != 0 && !abi.decode(returndata, (bool))) {
            revert SafeERC20FailedOperation(address(token));
        }
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/812404cee89cd91d46f6234d03451b07eebf2e35/contracts/utils/Address.sol#L70-L72

    function functionCall(address target, bytes memory data) internal returns (bytes memory) {
        return functionCallWithValue(target, data, 0);
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/812404cee89cd91d46f6234d03451b07eebf2e35/contracts/utils/Address.sol#L83-L89

    function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
        if (address(this).balance < value) {
            revert AddressInsufficientBalance(address(this));
        }
        (bool success, bytes memory returndata) = target.call{value: value}(data); ----------->> uses the low level call to puch the reward token.
        return verifyCallResultFromTarget(target, success, returndata);
    }

Above code flow is sufficient to understand this issue.

[141345]

The question here is not whether reentrancy is possible, but whether the admin will set some token with transfer hook as the reward. Most mainstream token does not have the hook, so the likelihood is quite low for this issue.

[aktech297]

Yes. The possibility is high.

I can say this by looking at the Perennial V1 contract reward token and how the implementation is done to claim the rewards.

Here is perennial statement about the reward token.

https://github.com/equilibria-xyz/perennial-mono/blob/a71759d63d817847e4f7ec49d9975a3fd575fdbb/packages/perennial/contracts/interfaces/types/ProgramInfo.sol#L23-L29

    /**
     * @dev Reward ERC20 token contract
     * @notice Perennial does not support non-standard ERC20s as reward tokens for incentive programs, including,
                but not limited to: fee on transfer and rebase tokens. Using such a non-standard token will likely
                result in loss of funds.
     */
    Token18 token;

Now, lets look at the claim reward functionality. https://github.com/equilibria-xyz/perennial-mono/blob/a71759d63d817847e4f7ec49d9975a3fd575fdbb/packages/perennial/contracts/incentivizer/Incentivizer.sol#L138-L144

The claim function has the nonReentrant check which is sufficient to say that the reward token would allow for reentrancy.

    function claim(IProduct product, uint256[] calldata programIds)
    external
    nonReentrant
    {
        _claimProduct(msg.sender, product, programIds);
    }

https://github.com/equilibria-xyz/perennial-mono/blob/a71759d63d817847e4f7ec49d9975a3fd575fdbb/packages/perennial/contracts/incentivizer/Incentivizer.sol#L181-L190

function _claimProduct(address account, IProduct product, uint256[] calldata programIds)
    private
    isProduct(product)
    notPaused
    settleForAccount(account, product)
    {
        for (uint256 i; i < programIds.length; i++) {
            _claimProgram(account, product, programIds[i]);
        }
    }

Also, when we follow the function flow, the state update is done prior to transferring the tokens. As a continuation, Perennial V2 could use the same reward token. I think, I have show enough evidence senior.... : )

[hrishibhat]

@141345 @aktech297 Currently, the readme mentions only DSU and USDC. How is this issue relevant to the above tokens?

[141345]

@141345 @aktech297 Currently, the readme mentions only DSU and USDC. How is this issue relevant to the above tokens?

Yes, the discussion is hypothetical that the admin change the reward token in the future. DSU and USDC do not have any problem.

[aktech297]

I am not sure that DSU and USDC are reward tokens. I searched for that and didn't find any reference. If you find any mention about reward token, can your pls share the link here. Also, the V1 mention that's reward token is a standard. But V2 mentioned about the USDC as non standard. May be you can check about the reward token with sponser. Its really dangerous to assume on the safer side on this , since there is real loss of asset involved. Also, in V2 , the reward token can be changed anytime. So, it's really hard to assume on this one. Another question is , why do the V1 has non re enetrant protection.?

[arjun-io]

The reward token could be any theoretical token the admin wants but we would consider them trusted since they're set by a trusted entity in the system. This would be similar to how a coordinator setting parameters in malicious ways is not a valid issue unless it can negatively affect other markets or the protocol overall.

[aktech297]

Looking for @hrishibhat final judging.. sponser mentioned any token with trusted.. so possibilities are hanging for both kind of token , i.e, tokens which allow re entrancy and tokens that will not.

[panprog]

I think it's pretty clear that expected reward token is standard ERC-20 token, which doesn't have any transfer hooks and thus can't be used to re-enter. If the admin sets a reward token which does allow re-entrancy, this is admin mistake and should be invalid.

[aktech297]

Posting here to everyone to know more about reentrancy.

https://ethereum.stackexchange.com/questions/146931/how-is-re-entrancy-attack-done-in-case-of-erc20-tokens-instead-of-eth#:~:text=Re%2Dentrancy%20attacks%20are%20possible%20whenever%20a%20smart%20contract%20calls%20an%20external%20target%2C%20and%20this%20is%20not%20limited%20to%20ETH%20transfer.%20It%20is%20possible%20to%20trigger%20a%20re%2Dentrancy%20attack%20through%20executing%20a%20method%20on%20an%20external%20target%20as%20well

[hrishibhat]

Result: Low Unique This is a low issue as there are no tokens mentioned in the readme that would consider this, in addition this would be admin mistake if such tokens are used. https://github.com/sherlock-audit/2023-07-perennial-judging/issues/135#issuecomment-1700986744

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• panprog: accepted
• aktech297: rejected