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

Verifying the staking instance can be bypassed #11

Closed
c4-bot-5 opened this issue Jun 16, 2024 · 1 comment
Closed

Verifying the staking instance can be bypassed #11

c4-bot-5 opened this issue Jun 16, 2024 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value invalid This doesn't seem right 🤖_primary AI based primary recommendation withdrawn by warden Special case: warden has withdrawn this submission and it can be ignored

Comments

@c4-bot-5
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingVerifier.sol#L206-L221

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Comment out the stakingToken function in MockStaking and add the following test to ServiceStakingFactory.js

Run with: npx hardhat test --grep "Test verification on no staking token function"

        it("Test verification on no staking token function", async function () {
            // Set the verifier
            await stakingFactory.changeVerifier(stakingVerifier.address);

            // Whitelist implementation
            await stakingVerifier.setImplementationsStatuses([staking.address], [true], true);

            initPayload = staking.interface.encodeFunctionData("initialize", [token.address]);
            
            await expect(
                stakingFactory.createStakingInstance(staking.address, initPayload)
            ).to.be.revertedWithCustomError(stakingFactory, "UnverifiedProxy");
        });

Tools Used

Manual Review, JS

Recommended Mitigation Steps

In StakingVerifier::vefiryInstance return false if the staticall fails

    function verifyInstance(address instance, address implementation) external view returns (bool) {
...
        bytes memory tokenData = abi.encodeCall(IStaking.stakingToken, ());
        (bool success, bytes memory returnData) = instance.staticcall(tokenData);

        // Check the returnData is the call was successful
        if (success) {
            // The returned size must be 32 to fit one address
            if (returnData.length == 32) {
                address token = abi.decode(returnData, (address));
                if (token != olas) {
                    return false;
                }
            } else {
                return false;
            }
+        } else {
+            return false;
+        }

        return true;
    }

Assessed type

Invalid Validation

@c4-bot-5 c4-bot-5 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 Jun 16, 2024
c4-bot-4 added a commit that referenced this issue Jun 16, 2024
@c4-bot-4 c4-bot-4 added invalid This doesn't seem right withdrawn by warden Special case: warden has withdrawn this submission and it can be ignored and removed bug Something isn't working labels Jun 16, 2024
@c4-bot-5
Copy link
Contributor Author

Withdrawn by yotov721

@c4-bot-13 c4-bot-13 added the 🤖_primary AI based primary recommendation label Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value invalid This doesn't seem right 🤖_primary AI based primary recommendation withdrawn by warden Special case: warden has withdrawn this submission and it can be ignored
Projects
None yet
Development

No branches or pull requests

3 participants