code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

The contract Controller.sol contains two functions "strategies", which have different functionality. So this is not a case of function overloading but of shadowing.

This is confusing and can lead to mistakes by future programmers.

Proof of Concept

https://github.com/code-423n4/2021-09-yaxis/blob/main/contracts/v3/controllers/Controller.sol#L548

function strategies(address _vault) external view returns (address[] memory) {
        return _vaultDetails[_vault].strategies;
    }

function strategies() external view override returns (uint256) {
        return _vaultDetails[msg.sender].strategies.length;
    }

Tools Used

Recommended Mitigation Steps

Rename the second strategies() to strategieslength()

Haz077 commented 3 years ago

I agree that both functions shouldn't have the same name to avoid confusion, but there will be no shadowing because they have different parameters.

GalloDaSballo commented 3 years ago

Agree with both sides of the argument, no shadowing here, but not ideal coding convention Downgrading to Non-Critical

loudoguno commented 3 years ago

reopening as primary issue for N-08 as per judge in findings sheet

code-423n4 / 2021-09-yaxis-findings