sherlock-audit / 2024-02-tapioca-judging

2 stars 2 forks source link

bin2chen - rebalance() Permission Control Error #52

Closed sherlock-admin4 closed 5 months ago

sherlock-admin4 commented 5 months ago

bin2chen

medium

rebalance() Permission Control Error

Summary

In the function rebalance(), the incorrect use of || prevents the rebalancer from enforcing permission restrictions.

Vulnerability Detail

Balancer.rebalance() The implementation is as follows::

    function rebalance(
        address payable _srcOft,
        uint16 _dstChainId,
        uint256 _slippage,
        uint256 _amount,
        bytes memory _ercData
    ) external payable onlyValidDestination(_srcOft, _dstChainId) onlyValidSlippage(_slippage) {
@>      if (msg.sender != owner() || msg.sender != rebalancer) revert NotAuthorized();

        if (connectedOFTs[_srcOft][_dstChainId].rebalanceable < _amount) {
            revert RebalanceAmountNotSet();
        }

        //extract
        ITOFT(_srcOft).extractUnderlying(_amount);

The code above contains an error where the logical OR (||) operator is misused. As a result, the condition for msg.sender requires msg.sender == owner == rebalancer in order to execute successfully.

However, in practice, rebalancer is not the same as owner; it is typically associated with the bot.

mTOFT contract has a function to allow rebalancing of assets, this is done off-chain by a Gelato bot. The caller address of mTOFT.extractUnderlying() needs to be whitelisted to perform the action.

function setRebalancer(address _addr) external onlyOwner {
rebalancer = _addr;
emit RebalancerUpdated(rebalancer, _addr);
}

the bot will be unable to execute the rebalance() function due to this flawed permission control.

Impact

Incorrect permission control, causing the bot to fail to pass the permission request

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/Balancer.sol#L176

Tool used

Manual Review

Recommendation

    function rebalance(
        address payable _srcOft,
        uint16 _dstChainId,
        uint256 _slippage,
        uint256 _amount,
        bytes memory _ercData
    ) external payable onlyValidDestination(_srcOft, _dstChainId) onlyValidSlippage(_slippage) {
-       if (msg.sender != owner() || msg.sender != rebalancer) revert NotAuthorized();
+       if (msg.sender != owner() && msg.sender != rebalancer) revert NotAuthorized();

        if (connectedOFTs[_srcOft][_dstChainId].rebalanceable < _amount) {
            revert RebalanceAmountNotSet();
        }

Duplicate of #89

sherlock-admin2 commented 5 months ago

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

WangAudit commented:

medium cause those addresses are set to the same on initialization; fair to admit they have high probability of being different; but due to being the same initially it's medium. Btw; checked the issue on Remix and it will indeed revert