sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

0xadrii - Malicious individual council members can challenge ALL transactions without restriction, making them able to prevent ANY transaction from being executed #142

Closed sherlock-admin closed 7 months ago

sherlock-admin commented 7 months ago

0xadrii

high

Malicious individual council members can challenge ALL transactions without restriction, making them able to prevent ANY transaction from being executed

Summary

All proposed transactions can be challenged by malicious council members, causing a denial of service in the TelcoinDistributor contract.

Vulnerability Detail

Malicious CouncilMember NFT holders are able to challenge all of the proposed transactions without limits.

As we can see, the challengeTransaction() found in TelcoinDistributor.sol simply allows any council member to challenge all the transactions they want:

function challengeTransaction( 
        uint256 transactionId
    ) external onlyCouncilMember whenNotPaused {
        // Makes sure the id exists
        require(
            transactionId < proposedTransactions.length,
            "TelcoinDistributor: Invalid index"
        );

        // Reverts if the current time exceeds the sum of the transaction's timestamp and the challenge period
        require(
            block.timestamp <=
                proposedTransactions[transactionId].timestamp + challengePeriod,
            "TelcoinDistributor: Challenge period has ended"
        );

        // Sets the challenged flag of the proposed transaction to true
        proposedTransactions[transactionId].challenged = true;

        // Emits an event with the transaction ID and the challenger's address
        emit TransactionChallenged(transactionId, _msgSender());
    }

As per the sponsor’s message in Discord: “The Council members are semi-trusted. These are people who have been elected and are expected to behave in their own best interest.”.

Impact

High. A council member can challenge ALL transactions proposed in the TelcoinDistributor, which breaks the contract’s main purpose and effectively DoS’es its functionality.

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/protocol/core/TelcoinDistributor.sol#L115C5-L136C6

Tool used

Manual Review

Recommendation

The contract’s design must be rethinked so that it implements a quorum-based approach to handle proposals. A good starting point would be using OpenZeppelin’s Governor contract, which implements the mentioned quorum-based approach.

amshirif commented 7 months ago

The reason this is not an issue is because of how this contract is meant to interact with the rest of the protocol. If a council member decides to be obstinate and reject everything they can do so, this might cause the community to decide to replace him. Meanwhile, anything that is rejected is elevated to a vote. This vote will consist of the same members, but will need a majority to pass, rather than a single dissection to negate. It will the execute directly from the safe associated with the funds.

nevillehuang commented 7 months ago

Invalid based on this reasoning by sponsor