ni8mare - _setTotal function in CrosschainDistributor will revert

[sherlock-admin2]

ni8mare

high

_setTotal function in CrosschainDistributor will revert

Summary

The _setTotal function in CrosschainDistributor will revert when the owner tries to set a new total value.

Vulnerability Detail

Once the total state variable is set in the constructor, it will not be possible to update it. This is because the function makes a call to the _allowConnext function which uses safeApprove to allow Connext to spend the ERC20 token on the contract's behalf. The problem lies in using safeApprove function. Check the implementation of the safeApprove function:

function safeApprove(
    IERC20 token,
    address spender,
    uint256 value
) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    require(
        (value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

The require statement specifies that a user is either setting their allowance to zero or setting it from zero. But, when the setToken function is called (after it has already been called in the constructor), the statement _allowConnext(total - claimed); which tries to approve a non-zero value would revert, as the required condition in the safeApprove function would fail. The safeApprove requires that we first set the allowance to 0 and then to a non-zero value. But, that is not the case here.

Impact

It would not be possible to set a new total value by the owner.

Code Snippet

https://github.com/SoftDAO/contracts/blob/291df55ddb0dbf53c6ed4d5b7432db0c357ca4d3/contracts/claim/abstract/CrosschainDistributor.sol#L40-L45

Tool used

Manual Review

Recommendation

It is recommended that the protocol just use the approve function here. This would not be a problem, cause we are only approving the Connext address which is trusted. Or they may make use of 'safeIncreaseAllowance' and 'safeDecreaseAllowance' functions.

Duplicate of #141