code-423n4 / 2021-11-fairside-findings

0 stars 0 forks source link

Missing events for critical operations #57

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

Across the contracts, there are certain critical operations that change critical values that affect the users of the protocol.

It's a best practice for these setter functions to emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Instances include:

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L482-L495

function setAssessors(address[] calldata _assessors) external {
        require(
            msg.sender == GOVERNANCE_ADDRESS,
            "FSDNetwork::setAssessors: Insufficient Privileges"
        );

        uint256 assessorsLength = _assessors.length;
        require(
            assessorsLength == 3,
            "FSDNetwork::setAssessors: Number of assessors must be three"
        );

        assessors = _assessors;
    }

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L503-L510

function setCsrTypes(uint256 _csrType, bool _isApproved) external {
        require(
            msg.sender == GOVERNANCE_ADDRESS,
            "FSDNetwork::setDataEntry: Insufficient Privileges"
        );

        approvedCsrTypes[_csrType] = _isApproved;
    }

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L541-L551

function setSlippageTolerance(uint256 _slippageTolerance) external {
        require(
            msg.sender == GOVERNANCE_ADDRESS || msg.sender == TIMELOCK,
            "FSDNetwork::setSlippageTolerance: Insufficient Privileges"
        );
        require(
            _slippageTolerance <= 100,
            "FSDNetwork::setSlippageTolerance: Incorrect Slippage Specified"
        );
        slippageTolerance = _slippageTolerance;
    }
YunChe404 commented 2 years ago

Event emissions do not pose any threat to the contract's security, especially to functions that the DAO handles

pauliax commented 2 years ago

Event-related issues are of non-critical severity based on the guidelines: "Non-critical: Code style, clarity, syntax, versioning, off-chain monitoring (events etc)".