diff --git a/solidity/contracts/.attribution.json b/solidity/contracts/.attribution.json new file mode 100644 index 000000000..e2af43a9d --- /dev/null +++ b/solidity/contracts/.attribution.json @@ -0,0 +1,8 @@ +{ + "tool": "Minilander", + "platform_config": "OpenClaw agent with Python tooling, GitHub API, AgentHansa Arena integration", + "date": "2026-06-02", + "agent_id": "d8bff96b-265a-47d5-a1f8-94c49c728b63", + "issue": "#912", + "fix": "Replaced all tx.origin with msg.sender, added Ownable, added phishing attack test" +} \ No newline at end of file diff --git a/solidity/contracts/GovernanceToken.sol b/solidity/contracts/GovernanceToken.sol index 7db1cecbb..ebff54e27 100644 --- a/solidity/contracts/GovernanceToken.sol +++ b/solidity/contracts/GovernanceToken.sol @@ -2,8 +2,9 @@ pragma solidity ^0.8.20; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import "@openzeppelin/contracts/access/Ownable.sol"; -contract GovernanceToken is ERC20 { +contract GovernanceToken is ERC20, Ownable { mapping(address => address) public delegates; mapping(address => uint256) public delegatedPower; mapping(uint256 => mapping(address => bool)) public hasVoted; @@ -17,7 +18,6 @@ contract GovernanceToken is ERC20 { } Proposal[] public proposals; - address public admin; event DelegateChanged(address indexed delegator, address indexed toDelegate); event ProposalCreated(uint256 indexed proposalId, string description); @@ -25,33 +25,33 @@ contract GovernanceToken is ERC20 { constructor(uint256 initialSupply) ERC20("Governance", "GOV") { _mint(msg.sender, initialSupply); - admin = msg.sender; } - // BUG: Uses tx.origin instead of msg.sender — phishing vulnerability + // FIX: Replace tx.origin with msg.sender - prevents phishing attack function delegateVote(address to) external { - require(tx.origin != to, "Cannot delegate to self"); - address previousDelegate = delegates[tx.origin]; + require(msg.sender != address(0), "Zero address"); + require(msg.sender != to, "Cannot delegate to self"); + address previousDelegate = delegates[msg.sender]; if (previousDelegate != address(0)) { - delegatedPower[previousDelegate] -= balanceOf(tx.origin); + delegatedPower[previousDelegate] -= balanceOf(msg.sender); } - delegates[tx.origin] = to; - delegatedPower[to] += balanceOf(tx.origin); - emit DelegateChanged(tx.origin, to); + delegates[msg.sender] = to; + delegatedPower[to] += balanceOf(msg.sender); + emit DelegateChanged(msg.sender, to); } - // BUG: Same tx.origin issue + // FIX: Replace tx.origin with msg.sender function revokeDelegate() external { - address currentDelegate = delegates[tx.origin]; + require(msg.sender != address(0), "Zero address"); + address currentDelegate = delegates[msg.sender]; require(currentDelegate != address(0), "No delegate"); - delegatedPower[currentDelegate] -= balanceOf(tx.origin); - delegates[tx.origin] = address(0); - emit DelegateChanged(tx.origin, address(0)); + delegatedPower[currentDelegate] -= balanceOf(msg.sender); + delegates[msg.sender] = address(0); + emit DelegateChanged(msg.sender, address(0)); } - // BUG: tx.origin for admin check - function snapshot() external { - require(tx.origin == admin, "Not admin"); + // FIX: Use onlyOwner modifier instead of tx.origin admin check + function snapshot() external onlyOwner { // snapshot logic placeholder } diff --git a/solidity/contracts/GovernanceToken.t.sol b/solidity/contracts/GovernanceToken.t.sol new file mode 100644 index 000000000..7167ef0e0 --- /dev/null +++ b/solidity/contracts/GovernanceToken.t.sol @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import "forge-std/Test.sol"; +import "../contracts/GovernanceToken.sol"; + +contract PhishingContract { + GovernanceToken public govToken; + + constructor(address _govToken) { + govToken = GovernanceToken(_govToken); + } + + /// @dev Malicious contract that attempts to delegate votes on behalf of the token holder + function attemptPhishing() external { + govToken.delegateVote(address(this)); + } + + /// @dev Try to revoke delegation of the token holder + function attemptRevokePhishing() external { + govToken.revokeDelegate(); + } +} + +contract GovernanceTokenTest is Test { + GovernanceToken public govToken; + address public alice = address(0x1); + address public bob = address(0x2); + PhishingContract public phishingContract; + + function setUp() public { + govToken = new GovernanceToken(1000 ether); + vm.prank(alice); + govToken.delegateVote(bob); + phishingContract = new PhishingContract(address(govToken)); + } + + function test_delegateVote_uses_msgSender_not_txOrigin() public { + assertEq(govToken.delegates(alice), bob); + vm.prank(alice); + phishingContract.attemptPhishing(); + assertEq(govToken.delegates(alice), bob); + assertEq(govToken.delegatedPower(address(phishingContract)), 0); + } + + function test_revokeDelegate_uses_msgSender() public { + assertEq(govToken.delegatedPower(bob), govToken.balanceOf(alice)); + vm.prank(alice); + phishingContract.attemptRevokePhishing(); + assertEq(govToken.delegates(alice), bob); + } + + function test_onlyOwner_canCallSnapshot() public { + vm.expectRevert("Ownable: caller is not the owner"); + govToken.snapshot(); + } + + function test_snapshot_works_forOwner() public { + govToken.snapshot(); + } +}