Curta Golf Bounty Contract

GolfBounty.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {ICurtaGolf} from "curta-golf/src/interfaces/ICurtaGolf.sol";
import {ICourse} from "curta-golf/src/interfaces/ICourse.sol";
import {IERC721} from "./interfaces/IERC721.sol";
import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";
import {SafeCastLib} from "solady/utils/SafeCastLib.sol";

/// @author philogy <https://github.com/philogy>
contract GolfBounty {
    using SafeTransferLib for address;
    using SafeCastLib for uint256;

    error NotExpired();
    error Expired();
    error NonexistentCourse();
    error DoesntMeetThreshold();
    error ThresholdAlreadyMet();
    error AlreadyPaidOut();
    error NothingToRefund();

    event Deposit(Key indexed key, address indexed owner, uint256 amount);
    event Refund(Key indexed key, address indexed owner, uint256 amount);
    event Claimed(Key indexed key, address indexed to, uint256 total);

    ICurtaGolf internal constant CURTA_GOLF = ICurtaGolf(0x8cCd70b1B74eA505dbA39d2D11C3aB6a2CB14A8c);

    struct Key {
        uint32 courseId;
        uint32 threshold;
        uint256 expiry;
    }

    struct Bounty {
        bool paidOut;
        uint128 total;
        mapping(address => uint256) deposits;
    }

    mapping(uint32 courseId => mapping(uint32 threshold => mapping(uint256 expiry => Bounty))) internal _bounties;

    function deposit(Key calldata key) external payable {
        _checkNotExpired(key);
        _checkCourseValidity(key);
        Bounty storage bounty = _getBounty(key);
        if (bounty.paidOut) revert AlreadyPaidOut();
        bounty.total += msg.value.toUint128();
        bounty.deposits[msg.sender] += msg.value;
        emit Deposit(key, msg.sender, msg.value);
    }

    function refund(Key calldata key) external {
        // Check that the bounty expired and wasn't empty.
        if (key.expiry > block.timestamp) revert NotExpired();
        Bounty storage bounty = _getBounty(key);
        if (bounty.paidOut) revert AlreadyPaidOut();
        uint256 amount = bounty.deposits[msg.sender];
        // Reset and send caller their deposit.
        bounty.deposits[msg.sender] = 0;
        emit Refund(key, msg.sender, amount);
        msg.sender.safeTransferETH(amount);
    }

    function claim(Key calldata key) external {
        _checkNotExpired(key);
        (, uint256 gasUsed,,) = CURTA_GOLF.getCourse(key.courseId);
        if (gasUsed >= key.threshold) revert DoesntMeetThreshold();
        // Send current holder of the KING token the total.
        Bounty storage bounty = _getBounty(key);
        if (bounty.paidOut) revert AlreadyPaidOut();
        bounty.paidOut = true;
        address to = IERC721(address(CURTA_GOLF)).ownerOf(key.courseId);
        uint256 total = bounty.total;
        emit Claimed(key, to, total);
        to.safeTransferETH(total);
    }

    function getBounty(Key calldata key) external view returns (uint256, bool) {
        Bounty storage bounty = _getBounty(key);
        return (bounty.total, bounty.paidOut);
    }

    function getDeposit(Key calldata key, address owner) public view returns (uint256) {
        return _getBounty(key).deposits[owner];
    }

    function _checkNotExpired(Key calldata key) internal view {
        if (key.expiry <= block.timestamp) revert Expired();
    }

    function _checkCourseValidity(Key calldata key) internal view {
        (ICourse setCourse, uint32 gasUsed,,) = CURTA_GOLF.getCourse(key.courseId);
        if (address(setCourse) == address(0)) revert NonexistentCourse();
        if (gasUsed < key.threshold) revert ThresholdAlreadyMet();
    }

    function _getBounty(Key calldata key) internal view returns (Bounty storage) {
        return _bounties[key.courseId][key.threshold][key.expiry];
    }
}

EDIT: old text struck through

This is a great idea!

At risk of being called a golf heretic, I think it's clearer to use a Key struct and hash pack it as the mapping key. The real-world cost is negligible on Base, but it greatly improves readability (making it easier to review / find bugs). Other than this it LGTM (tests aside).

Questions:

1. Do you think it needs the check for a valid course that I've added to increase()? Now that I think about it, they can just wait for expiry and then refund().
2. Is there ever a scenario where the bounty owner isn't just msg.sender? It's just a step where someone can carelessly lock a bounty permanently.

/// @author philogy <https://github.com/philogy>
contract GolfBounty {
    using SafeTransferLib for address;

    error NonExistentCourse();
    error NotExpired();
    error Expired();
    error DoesntMeetThreshold();

    ICurtaGolf internal constant CURTA_GOLF = ICurtaGolf(0x8cCd70b1B74eA505dbA39d2D11C3aB6a2CB14A8c);

    struct Key {
        address owner;
        uint32 courseId;
        uint32 expiry;
        uint32 gasThreshold;
    }

    struct Bounty {
        uint256 total;
    }

    mapping(bytes32 => Bounty) internal _bounties;

    function increase(Key memory key) external payable {
        (ICourse course,,,) = CURTA_GOLF.getCourse(key.courseId);
        if (address(course) == address(0)) {
            revert NonExistentCourse();
        }
        _bounties[_mappingKey(key)].total += msg.value;
    }

    function refund(Key memory key) external {
        if (key.expiry > block.timestamp) {
            revert NotExpired();
        }
        _emptyBountyTo(key.owner, key);
    }

    function claim(Key memory key) external {
        if (block.timestamp >= key.expiry) {
            revert Expired();
        }
        (, uint256 gasUsed,,) = CURTA_GOLF.getCourse(key.courseId);
        if (gasUsed >= key.gasThreshold) {
            revert DoesntMeetThreshold();
        }

        address owner = IERC721(address(CURTA_GOLF)).ownerOf(key.courseId);
        _emptyBountyTo(owner, key);
    }

    function _emptyBountyTo(address to, Key memory key) internal {
        Bounty storage bounty = _bounties[_mappingKey(key)];
        uint256 amount = bounty.total;
        delete bounty.total;
        to.safeTransferETH(amount);
    }

    function getTotal(Key memory key) external view returns (uint256) {
        return _bounties[_mappingKey(key)].total;
    }

    function _mappingKey(Key memory key) internal pure returns (bytes32) {
        return bytes32(abi.encodePacked(key.owner, key.courseId, key.expiry, key.gasThreshold));
    }
}

Appreciate the feedback @aschlosberg ! I made some changes to improve the claim UX, please review it as well if you have a minute 🙏

Main reason I packed the key and used it that way is for calldata compression. Calldata is one of the primary costs on L2s so avoiding the unnecessary ABI-padding for 4 (now 3) parameters that could be packed into a single word probably saves quite a bit.

no events emitted for deposit/claim/refund.

L19 OWNER_OFFSET can be removed.

L22 GAS_THRESHOLD_OFFSET = 0; and shifts by it can be removed as it's no op.

in deposit:
no expiry check (require expiry > block.timestamp)
no check course id is valid (require getCourse.course != address(0))
no check whether solution already exists (require getCourse.gasUsed > threshold)
--> med risk: the solution might be concidentally submitted the same time bounty being deposited. if bounty is included later in the block (or in the next blocks), it can still be claimed even though the bounty was submitted after a solution was found. so best to prevent depositing bounties when a solution already exists.

bounty.total == 0 is applied when a claim occurs, signaling no more bounty exists. However it also signals uninitialized/unused bounty. Since there is no other state variables to check if bounty has been claimed or initialized, there might be confusion if the variable is dependend externally.

blobs got our back, no need to confuse people with a packed variable. if you want to satisfy the optimizoor inside you, you can add a fallback function with custom encoding handling.

probably an inevitable issue: the person who breaks the threshold is not guaranteed to claim the bounty if the solution is not submitted atomically as bounty is claimed. i.e. someone can find a microoptimization and become the king and claim the bounty before the person who initially broke the threshold.

gonna raise the med risk one to high cuz realized other depositors who got their bounty claimed can steal the new depositor by calling refund.

A deposits 50
B solves and claims 50 from A
C coincidentally deposits 100 right after bounty was claimed.
A refunds, stealing from C 50

New version doesn't fully fix the main issue. Because the newly added bounty.paidOut checks in refund() and claim() not only prevent stealing late depositor funds, they also lock the funds which get deposited after a claim. So it should simply prevent depositing in after a claim. To be extra safe, I would prevent depositing when threshold is already met regardless of bounty being claimed or not. I suggest the fix below.

diff --git a/GolfBounty.sol b/GolfBounty-update.sol
index 3a1b575..6c08da9 100644
--- a/GolfBounty.sol
+++ b/GolfBounty-update.sol
@@ -16,6 +16,7 @@ contract GolfBounty {
     error Expired();
     error NonexistentCourse();
     error DoesntMeetThreshold();
+    error ThresholdAlreadyMet();
     error AlreadyPaidOut();
     error NothingToRefund();
 
@@ -41,7 +42,7 @@ contract GolfBounty {
 
     function deposit(Key calldata key) external payable {
         _checkNotExpired(key);
-        _checkExistingCourse(key);
+        _checkCourseValidity(key);
         Bounty storage bounty = _getBounty(key);
         bounty.total += msg.value.toUint128();
         bounty.deposits[msg.sender] += msg.value;
@@ -87,9 +88,10 @@ contract GolfBounty {
         if (key.expiry <= block.timestamp) revert Expired();
     }
 
-    function _checkExistingCourse(Key calldata key) internal view {
-        (ICourse setCourse,,,) = CURTA_GOLF.getCourse(key.courseId);
+    function _checkCourseValidity(Key calldata key) internal view {
+        (ICourse setCourse, uint256 gasUsed,,) = CURTA_GOLF.getCourse(key.courseId);
         if (address(setCourse) == address(0)) revert NonexistentCourse();
+        if (gasUsed < key.threshold) revert ThresholdAlreadyMet();
     }
 
     function _getBounty(Key calldata key) internal view returns (Bounty storage) {

Other than this LGTM.