<div align="center">
<center>
<img src="https://avatars.githubusercontent.com/u/167952721" height="350" alt="@offbeatsecurity" style="margin-bottom: 20px;">
<h1>Doge Lock Review</h1>
<h3>January 7, 2025</h3>
<p>Prepared for GOAT Network</p>
<p>Conducted by:</p>
<p>Richie Humphrey (devtooligan)</p>
<p>Kurt Willis (phaze)</p>
</center>
</div>

## About the **GOAT Network Doge Lock Review**
The GOAT Network Doge Lock protocol enables users to lock and bridge Dogecoin tokens between BNB Chain and GOAT Network using LayerZero's OFT (Omnichain Fungible Token) standard. The system consists of a locking contract that manages DOGE deposits and withdrawals, with associated bridging functionality through LayerZero's infrastructure. Users who lock their DOGE tokens can earn rewards based on their lock duration and amount, with rewards calculated off-chain.

## About **Offbeat Security**
Offbeat Security is a boutique security company providing unique security solutions for complex and novel crypto projects. Our mission is to elevate the blockchain security landscape through invention and collaboration.

## Summary & Scope
The [contracts](https://github.com/GOATNetwork/DogeLock/tree/dddaf6c18d05e0a4dbef5c7b605226830353c089/) folder was reviewed at commit [dddaf6c](https://github.com/GOATNetwork/DogeLock/tree/dddaf6c18d05e0a4dbef5c7b605226830353c089/).

The following **4 contracts** were in scope:
- contracts/DogeAdapterUpgradeable.sol
- contracts/DogeLockUpgradeable.sol
- contracts/GoatOFT.sol
- contracts/UpgradeableProxy.sol

The protocol implements a token locking and bridging mechanism through a combination of upgradeable contracts and LayerZero's OFT framework. The system uses configurable limits for individual and total locked amounts while providing bridging capabilities between chains.

The review identified 2 INFORMATIONAL severity issues. Key findings include more robust design patterns and clearer documentation. Additional recommendations focus on improving contract integration and optimizing gas usage.

| Identifier | Title | Severity | Fixed |
| ----------------------------------------------------------------------------------- | --------------------------------------------------------------------- | -------- | ----- |
| [I-01](#I-01-Token-decimals-should-be-fetched-from-token-contract-instead-of-hardcoding) | Token decimals should be fetched from token contract instead of hardcoding | Info | [Fixed](https://github.com/GOATNetwork/DogeLock/commit/d01c1a17a2718fedcadeb6b72ca85e51ce2d062e) |
| [I-02](#I-02-Minimum-balance-validation-doesnt-match-documented-behavior) | Minimum balance validation doesn't match documented behavior | Info | [Fixed](https://github.com/GOATNetwork/DogeLock/commit/d01c1a17a2718fedcadeb6b72ca85e51ce2d062e) |

## Protocol Assumptions & Design Notes
### Direct Bridge Interaction
The protocol allows users to interact with the bridge (OFTAdapter) directly without going through the DogeLockUpgradeable contract. This is an intentional design choice that provides flexibility for users who want to bridge tokens without participating in the locking mechanism. Users who choose this path will not receive rewards or other benefits associated with the locking contract.

## Additional Recommendations
### Centralization
The protocol uses an upgradeable proxy pattern which grants the contract owner significant control over the system through the ability to upgrade contract logic. The documentation does not specify the ownership structure, timelock controls, or governance processes around upgrades. This centralized control represents a trust requirement that users should be aware of.

### Code Quality

1. **Add Events for State Changes**

Add event emissions for state-changing functions to improve transparency and allow better tracking of contract changes:

contract DogeLockUpgradeable is IDogeLock, OwnableUpgradeable {
+   event MaxLockAmountSet(uint256 newAmount);
+   event PersonalLimitsSet(uint256 newMax, uint256 newMin);

    function setMax(uint256 _amount) external onlyOwner {
        require(_amount >= totalBalance, InvalidAmount());
        maxLockAmount = _amount;
+       emit MaxLockAmountSet(_amount);
    }

    function setPersonalLimit(uint256 _max, uint256 _min) external onlyOwner {
        require(_max > _min, InvalidAmount());
        personalMaxLockAmount = _max;
        personalMinLockAmount = _min;
+       emit PersonalLimitsSet(_max, _min);
    }
}

2. **Remove Redundant Balance Checks**

The `totalBalance` check is redundant in both `lock()` and `unlock()` functions as these values are already tracked through individual balances:

    function lock(uint256 _amount) external {
        require(_amount >= personalMinLockAmount, BelowMin());
        balances[msg.sender] += _amount;
        totalBalance += _amount;
        require(balances[msg.sender] <= personalMaxLockAmount, ExceededPersonalMax(balances[msg.sender]));
        require(totalBalance <= maxLockAmount, ExceededTotalMax(totalBalance));
-       require(_amount <= totalBalance, ExceededTotalBalance(totalBalance));
        dogeCoin.safeTransferFrom(msg.sender, address(this), _amount);
        emit Lock(msg.sender, _amount, block.number);
    }

    function unlock(uint256 _amount) external {
        require(_amount <= balances[msg.sender], ExceededBalance(balances[msg.sender]));
-       require(_amount <= totalBalance, ExceededTotalBalance(totalBalance));
        balances[msg.sender] -= _amount;
        totalBalance -= _amount;
        dogeCoin.safeTransfer(msg.sender, _amount);
        emit Unlock(msg.sender, _amount, block.number);
    }

3. **Move Zero-Address Check to Constructor**

Move the adapter address validation to the constructor to save gas and improve code clarity:

    constructor(address _dogeCoin, address _dogeAdapter) {
+       require(_dogeAdapter != address(0), InvalidAddress());
        dogeCoin = IERC20(_dogeCoin);
        dogeAdapter = _dogeAdapter;
    }

    function bridge(SendParam calldata _sendParam, MessagingFee calldata _fee) external payable {
        require(_fee.lzTokenFee == 0, PaymentNotSupported());
-       require(dogeAdapter != address(0), InvalidAddress());
        uint256 amount = _sendParam.amountLD;
        require(amount <= balances[msg.sender], ExceededBalance(balances[msg.sender]));
        require(amount <= totalBalance, ExceededTotalBalance(totalBalance));
        dogeCoin.approve(dogeAdapter, amount);
        (, OFTReceipt memory receipt) = IOFT(dogeAdapter).send{ value: msg.value }(_sendParam, _fee, msg.sender);
        amount = receipt.amountSentLD;
        balances[msg.sender] -= amount;
        totalBalance -= amount;
        emit Unlock(msg.sender, amount, block.number);
        emit Bridge(msg.sender, amount, _sendParam);
    }

4. **Use Infinite Approval for Adapter**

Set unlimited approval for the adapter in the initialize function to save gas on subsequent bridge operations:

    function initialize(address _owner) external initializer {
        __Ownable_init(_owner);
        maxLockAmount = 5_000_000 * DOGE_DECIMAL;
        personalMaxLockAmount = 50_000 * DOGE_DECIMAL;
        personalMinLockAmount = 50 * DOGE_DECIMAL;
+       dogeCoin.approve(dogeAdapter, type(uint256).max);
    }

    function bridge(SendParam calldata _sendParam, MessagingFee calldata _fee) external payable {
        require(_fee.lzTokenFee == 0, PaymentNotSupported());
        uint256 amount = _sendParam.amountLD;
        require(amount <= balances[msg.sender], ExceededBalance(balances[msg.sender]));
        require(amount <= totalBalance, ExceededTotalBalance(totalBalance));
-       dogeCoin.approve(dogeAdapter, amount);
        (, OFTReceipt memory receipt) = IOFT(dogeAdapter).send{ value: msg.value }(_sendParam, _fee, msg.sender);
        amount = receipt.amountSentLD;
        balances[msg.sender] -= amount;
        totalBalance -= amount;
        emit Unlock(msg.sender, amount, block.number);
        emit Bridge(msg.sender, amount, _sendParam);
    }

5. **Add Combined Lock and Bridge Function**

Add a new function that combines bridging and locking operations for better UX:

    function lockAndBridge(uint256 _lockAmount, SendParam calldata _sendParam, MessagingFee calldata _fee) external payable {
        lock(_lockAmount);
        bridge(_sendParam, _fee);
    }

## Detailed Findings

## Informational Findings

### [I-01] Token decimals should be fetched from token contract instead of hardcoding

#### Description
The DogeLockUpgradeable contract hardcodes the decimals of the Dogecoin token to `100_000_000` (8 decimals). While this matches the current decimals of Dogecoin, hardcoding such values can lead to errors if the value is incorrect or if the contract needs to be deployed for different tokens.

#### Recommendation
Fetch the decimals from the token contract during construction and store it in an immutable variable.

- uint256 constant DOGE_DECIMAL = 100_000_000;
+ uint256 immutable DOGE_DECIMAL;

constructor(address _dogeCoin, address _dogeAdapter) {
    dogeCoin = IERC20(_dogeCoin);
    dogeAdapter = _dogeAdapter;
+   DOGE_DECIMAL = 10 ** IERC20Metadata(_dogeCoin).decimals();
}

### [I-02] Minimum balance validation doesn't match documented behavior

#### Description
The `lock()` function's documentation states that "the final amount cannot be less than the personal min", but the implementation checks the input amount instead of the resulting balance. This inconsistency could cause confusion and fails to enforce the documented constraint.

/**
 * @dev The final amount cannot be less than the personal min or more than personal/total max.
 */
function lock(uint256 _amount) external {
    require(_amount >= personalMinLockAmount, BelowMin());
    balances[msg.sender] += _amount;
    totalBalance += _amount;
    require(balances[msg.sender] <= personalMaxLockAmount, ExceededPersonalMax(balances[msg.sender]));
    require(totalBalance <= maxLockAmount, ExceededTotalMax(totalBalance));
    dogeCoin.safeTransferFrom(msg.sender, address(this), _amount);
    emit Lock(msg.sender, _amount, block.number);
}

#### Recommendation
Either update the comment to reflect the current behavior or modify the validation to check final balances. If checking final balances, include an exception for zero balances to allow complete withdrawals.