Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Low level call returns true if the address doesn't exist #301

Closed
code423n4 opened this issue May 25, 2022 · 3 comments
Closed

Low level call returns true if the address doesn't exist #301

code423n4 opened this issue May 25, 2022 · 3 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/BoosterOwner.sol#L187
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/StashFactoryV2.sol#89
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/StashFactoryV2.sol#95
https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/StashFactoryV2.sol#101

Vulnerability details

As written in the documentation, the low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed. The low-level function call is used in some places in the code and it can be problematic. For example, in the execute of the BoosterOwner contract there is a low level call in order to call the target function, but if the given _to address doesn't exist success will be equal to true and the function will return true and the code execution will be continued like the call was successful.

function execute(
    address _to,
    uint256 _value,
    bytes calldata _data
) external onlyOwner returns (bool, bytes memory) {
    require(_to != booster, "!invalid target");

    (bool success, bytes memory result) = _to.call{value:_value}(_data);

    return (success, result);
}

Another place that this can happen is in the CreateStash function of the StashFactoryV2 contract, where there are low levels calls when calling the IsV1, IsV2 and IsV3 functions.

function CreateStash(uint256 _pid, address _gauge, address _staker, uint256 _stashVersion) external returns(address){
    require(msg.sender == operator, "!authorized");

    if(_stashVersion == uint256(3) && IsV3(_gauge)){
        //v3
        require(v3Implementation!=address(0),"0 impl");
        address stash = IProxyFactory(proxyFactory).clone(v3Implementation);
        IStash(stash).initialize(_pid,operator,_staker,_gauge,rewardFactory);
        emit StashCreated(stash, _stashVersion);
        return stash;
    }else if(_stashVersion == uint256(1) && IsV1(_gauge)){
        //v1
        require(v1Implementation!=address(0),"0 impl");
        address stash = IProxyFactory(proxyFactory).clone(v1Implementation);
        IStash(stash).initialize(_pid,operator,_staker,_gauge,rewardFactory);
        emit StashCreated(stash, _stashVersion);
        return stash;
    }else if(_stashVersion == uint256(2) && !IsV3(_gauge) && IsV2(_gauge)){
        //v2
        require(v2Implementation!=address(0),"0 impl");
        address stash = IProxyFactory(proxyFactory).clone(v2Implementation);
        IStash(stash).initialize(_pid,operator,_staker,_gauge,rewardFactory);
        emit StashCreated(stash, _stashVersion);
        return stash;
    }
    bool isV1 = IsV1(_gauge);
    bool isV2 = IsV2(_gauge);
    bool isV3 = IsV3(_gauge);
    require(!isV1 && !isV2 && !isV3,"stash version mismatch");
    return address(0);
}

function IsV1(address _gauge) private returns(bool){
    bytes memory data = abi.encode(rewarded_token);
    (bool success,) = _gauge.call(data);
    return success;
}

function IsV2(address _gauge) private returns(bool){
    bytes memory data = abi.encodeWithSelector(reward_tokens,uint256(0));
    (bool success,) = _gauge.call(data);
    return success;
}

function IsV3(address _gauge) private returns(bool){
    bytes memory data = abi.encodeWithSelector(rewards_receiver,address(0));
    (bool success,) = _gauge.call(data);
    return success;
}

If the _gauge address doesn't exist, the call will return true and the function will act like the address exists and true is supposed to be returned.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 25, 2022
code423n4 added a commit that referenced this issue May 25, 2022
@0xMaharishi 0xMaharishi added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels May 28, 2022
@0xMaharishi
Copy link

Gauge addresses are checked upstream so shouldn't be an issue

@0xMaharishi 0xMaharishi added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label May 30, 2022
@0xMaharishi
Copy link

@dmvt dmvt added invalid This doesn't seem right and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jul 11, 2022
@dmvt
Copy link
Collaborator

dmvt commented Jul 11, 2022

I didn't catch this initially, but the issue is not possible due to upstream guards per the comment by @kirk-baird here: #364 (comment)

@dmvt dmvt closed this as completed Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants