QA Report

[code423n4]

Summary of Findings

Severity : Low
Issue #1 : Possibility of stale permissions in signing for old Owner in EasySign.sol
Issue #2 : Need to emit an event in function execute() in VoteProxy.sol

Details Issue#1

Title : Possibility of stale permissions in signing for old Owner in EasySign.sol

Impact

Stale permissions in signing for old Owner can cause malicious behavior.

In the EasySign.sol contract, as part of the constructor, the owner/deployer is whitelisted in the approvedTeam structure.

constructor() {
    approvedTeam[msg.sender] = true;
}

In the event of transfer of ownership of this Contract, the old owner will still be able to sign the winningSignature.
Unless explicitly disabled via the modifyTeam function. This is a possibility if a proper process is not followed.
This bug is to mitigate such a possibility by explicitly adding relevant code.

Proof of Concept

Contract : EasySign.sol
Function : isWinningSignature()

function isWinningSignature(bytes32 _hash, bytes memory _signature)
    external
    view
    override
    returns (bool)
{
    address signer = recover(_hash, _signature);
    return approvedTeam[signer];
}

Recommended Mitigation Steps

Explicitly disable signing permission of old Owner, and enable signing permission to newOwner while a new owner is set.

Override the transferOwnership function in Ownable.sol with additional code

function transferOwnership(address newOwner) public override onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    modifyTeam(owner(), false);
    modifyTeam(newOwner, true);
    _transferOwnership(newOwner);
}

Details Issue#2

Title : Need to emit an event in function execute() in VoteProxy.sol

Impact

Need tracking of what command the owner executes in the VoteProxy.sol

Proof of Concept

Contract : VoteProxy.sol
Function : execute()

function execute(
    address _to,
    uint256 _value,
    bytes calldata _data
) external onlyOwner returns (bool, bytes memory) {
    (bool success, bytes memory result) = _to.call{value: _value}(_data);
    return (success, result);
}

Recommended Mitigation Steps

Add an event in function execute(). Sample given below, more fields can be added as required.

event Executed(address _to, uint256 _value, bool success);

function execute(
    address _to,
    uint256 _value,
    bytes calldata _data
) external onlyOwner returns (bool, bytes memory) {
    (bool success, bytes memory result) = _to.call{value: _value}(_data);
    Executed(_to, _value, success);
    return (success, result);
}

[GalloDaSballo]

Disagree with first finding, it's based on speculation about any role based system

Disagree with second finding as well, you don't necessarily have to emit an event for everything you do

[GalloDaSballo]

0