sherlock-audit / 2024-06-mellow-judging

4 stars 1 forks source link

pwning_dev - Array Indexing in acceptProposal #307

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

pwning_dev

High

Array Indexing in acceptProposal

Summary

Vulnerability Detail

function acceptProposal(uint256 index) external onlyAcceptor {
    if (index <= latestAcceptedNonce || _proposals.length < index)
        revert Forbidden();
    Proposal memory proposal = _proposals[index - 1];
    proxyAdmin.upgradeAndCall(
        proxy,
        proposal.implementation,
        proposal.callData
    );
    latestAcceptedNonce = index;
    emit ProposalAccepted(index, tx.origin);
}

The line Proposal memory proposal = _proposals[index - 1]; accesses _proposals array with index - 1. If index is 0 or exceeds _proposals.length, it may lead to out-of-bounds access, causing the contract to revert unexpectedly.

Alice proposes multiple upgrades to the dApp by calling propose function with valid implementation addresses and call data. The proposals are stored in the _proposals array.

Alice, acting as the acceptor, decides to accept a proposal by calling acceptProposal function with an index.

The Malicious Attacker monitors the blockchain and notices that Alice has proposed several upgrades. The Attacker attempts to exploit the contract by calling acceptProposal with an invalid or out-of-bounds index.

The Malicious Attacker submits a transaction calling acceptProposal with index greater than _proposals.length. Inside acceptProposal, the line Proposal memory proposal = _proposals[index - 1]; attempts to access _proposals at an invalid index. Due to the out-of-bounds access, the Solidity runtime reverts the transaction.

The transaction initiated by the Malicious Attacker fails, causing a revert on-chain. Alice, expecting the proposal to be accepted, encounters an unexpected failure without clear feedback from the contract

Impact

Disruption of Upgrade Process: The attempt by the Malicious Attacker disrupts the normal functioning of the AdminProxy contract.

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/security/AdminProxy.sol#L135C1-L147C1

Tool used

Manual Review

Recommendation

Bounds Checking: Implement robust bounds checking in the acceptProposal function to ensure that index is within valid range before accessing _proposals[index - 1]. Error Handling: Use appropriate error handling mechanisms (revert with a clear error message) to inform users about invalid inputs or out-of-bounds accesses.