Skip to content

review fixes #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 17, 2025
Merged

review fixes #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b15f143
fix: sb sign
ameeshaagrawal Nov 14, 2025
3f204c7
fix: review fixes
ameeshaagrawal Nov 14, 2025
2595862
fix: remove extra functions
ameeshaagrawal Nov 14, 2025
3d37ef4
Merge branch 'phase-1' into fix/review
ameeshaagrawal Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 29 additions & 46 deletions contracts/evmx/fees/GasAccountManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,15 @@ abstract contract GasAccountManagerStorage is IGasAccountManager {
uint256[50] _gap_after;
}

contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl, AppGatewayBase, Initializable {
contract GasAccountManager is
GasAccountManagerStorage,
Ownable,
AccessControl,
AppGatewayBase,
Initializable
{
using OverrideParamsLib for OverrideParams;
error OnlyPayloadConsumer();

constructor() {
_disableInitializers();
Expand All @@ -66,8 +73,7 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,
// ============ GAS ACCOUNT OPERATIONS ============

/// @notice Wrap native tokens into SGAS
// todo: remove onlywatcher
function wrapToGas(address receiver) external payable override onlyWatcher {
function wrapToGas(address receiver) external payable override {
uint256 amount = msg.value;
if (amount == 0) revert InvalidAmount();

Expand All @@ -80,8 +86,7 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,
}

/// @notice Unwrap SGAS to native tokens
// todo: remove onlywatcher
function unwrapFromGas(uint256 amount, address receiver) external onlyWatcher {
function unwrapFromGas(uint256 amount, address receiver) external {
// todo: use isGasAvailable, check all gasAccountToken__().balanceOf instances
if (gasAccountToken__().balanceOf(msg.sender) < amount) revert InsufficientGasAvailable();

Expand All @@ -98,33 +103,30 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,

/// @notice Deposit tokens from a chain into gas account
/// @dev Called by watcher after detecting GasStation deposit
function depositFromChain(bytes memory payload_) external onlyWatcher {
// Decode payload: (chainSlug, token, receiver, gasAmount, nativeAmount)
(
uint32 chainSlug, // todo: read chainslug from watcher instead of passing in payload
address token,
address depositTo,
uint256 gasAmount,
uint256 nativeAmount
) = abi.decode(payload_, (uint32, address, address, uint256, uint256));

tokenOnChainBalances[chainSlug][toBytes32Format(token)] += gasAmount + nativeAmount;
function depositFromChain(
address token_,
address depositTo_,
uint256 gasAmount_,
uint256 nativeAmount_
) external onlyWatcher {
uint32 chainSlug = watcher__().triggerFromChainSlug();
tokenOnChainBalances[chainSlug][toBytes32Format(token_)] += gasAmount_ + nativeAmount_;

// Mint tokens to the user
gasAccountToken__().mint(depositTo, gasAmount);
if (nativeAmount > 0) {
gasAccountToken__().mint(depositTo_, gasAmount_);
if (nativeAmount_ > 0) {
// if native transfer fails, add to gas
bool success = gasVault__().withdraw(depositTo, nativeAmount);
bool success = gasVault__().withdraw(depositTo_, nativeAmount_);

if (!success) {
// Convert failed native amount to gas
gasAccountToken__().mint(depositTo, nativeAmount);
gasAmount += nativeAmount;
nativeAmount = 0;
gasAccountToken__().mint(depositTo_, nativeAmount_);
gasAmount_ += nativeAmount_;
nativeAmount_ = 0;
}
}

emit Deposited(chainSlug, token, depositTo, gasAmount, nativeAmount);
emit Deposited(chainSlug, token_, depositTo_, gasAmount_, nativeAmount_);
}
Comment on lines +106 to 130
Copy link

@coderabbitai coderabbitai bot Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Make depositFromChain return the payload id (or stop exposing it).

Line 107: GasStation now invokes this through an interface that returns bytes memory, but this implementation returns nothing. The ABI decoder therefore reverts on every call because it expects dynamic return data. This should return the payload identifier you want to emit (and be marked override accordingly), or the caller must stop decoding a value. As written, any on-chain deposit flow breaks.

🤖 Prompt for AI Agents 
In contracts/evmx/fees/GasAccountManager.sol around lines 106-130, the
depositFromChain function currently has no return but the external
interface/consumer expects a bytes (dynamic) return value causing ABI decode
reverts; change the function signature to return bytes memory (and add override
if required by the interface), compute/derive the payload identifier you intend
to emit (or reuse the emitted identifier) and return it at the end of the
function, and update the contract/interface declarations so ABI types match (or
alternatively remove the return from the interface/caller if you prefer not to
expose it).


/// @notice Withdraw SGAS to tokens on another chain
Expand Down Expand Up @@ -205,27 +207,6 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,
gasAccountToken__().mint(transmitter, amount);
}

/// @notice Get available gas balance for an account
/// @dev Returns balance minus escrowed amount
function availableGas(address account) external view override returns (uint256) {
return gasAccountToken__().balanceOf(account);
}

/// @notice Get total gas balance including escrowed
function totalGas(address account) external view override returns (uint256) {
return gasAccountToken__().totalBalanceOf(account);
}

/// @notice Get currently escrowed gas for an account
function escrowedGas(address account) external view override returns (uint256) {
return gasEscrow__().getEscrowedAmount(account);
}

/// @notice Approve an app to spend gas from your account
function approveGasSpending(address app, uint256 amount) external override {
gasAccountToken__().approve(app, amount);
}

function setGasStation(uint32 chainSlug_, bytes32 gasStation_) external onlyOwner {
gasStations[chainSlug_] = gasStation_;
emit GasStationSet(chainSlug_, gasStation_);
Expand All @@ -236,7 +217,6 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,
emit ForwarderSolanaSet(forwarderSolana_);
}


function setGasStationSolanaProgramId(bytes32 gasStationSolanaProgramId_) external onlyOwner {
gasStationSolanaProgramId = gasStationSolanaProgramId_;
emit GasStationSolanaProgramIdSet(gasStationSolanaProgramId_);
Expand Down Expand Up @@ -268,8 +248,11 @@ contract GasAccountManager is GasAccountManagerStorage, Ownable, AccessControl,
overrideParams = overrideParams.setMaxFees(fees_);
}

// todo: auth consumefrom check
function increaseFees(bytes32 payloadId_, uint256 newMaxFees_) public {
if (msg.sender != watcher__().getPayload(payloadId_).consumeFrom)
revert OnlyPayloadConsumer();

// fees deducted from consumeFrom account
_increaseFees(payloadId_, newMaxFees_);
}

Expand Down
5 changes: 1 addition & 4 deletions contracts/evmx/fees/GasAccountToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ contract GasAccountToken is ERC20, Ownable, Initializable {
uint256 amount_
) public override returns (bool) {
if (!isGasAvailable(from_, msg.sender, amount_)) revert InsufficientGasAvailable();

// todo: check
if (msg.sender == address(addressResolver__.watcher__()))
_approve(from_, msg.sender, amount_);
return super.transferFrom(from_, to_, amount_);
Expand All @@ -103,8 +101,7 @@ contract GasAccountToken is ERC20, Ownable, Initializable {
) public view returns (bool) {
// If consumeFrom_ is not same as spender_ or spender_ is not watcher, check if it is approved
if (spender_ != address(addressResolver__.watcher__()) && consumeFrom_ != spender_) {
// todo: amount check instead of zero check
if (allowance(consumeFrom_, spender_) == 0) return false;
if (allowance(consumeFrom_, spender_) < amount_) return false;
}

return balanceOf(consumeFrom_) >= amount_;
Expand Down
3 changes: 1 addition & 2 deletions contracts/evmx/fees/GasEscrow.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,10 @@ contract GasEscrow is IGasEscrow, GasEscrowStorage, Initializable, Ownable {
payloadEscrow[payloadId_] = EscrowEntry({
account: consumeFrom_,
amount: amount,
timestamp: block.timestamp, // todo: needed?
state: EscrowState.Active
});

emit GasEscrowed(payloadId_, consumeFrom_, amount); // todo: emit diff amount not total, diff applies to both accountEscrow and EscrowEntry.
emit GasEscrowed(payloadId_, consumeFrom_, amount_);
}

/// @notice Release escrow back to account, cases where payload is not executed
Expand Down
1 change: 0 additions & 1 deletion contracts/evmx/helpers/AsyncPromise.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ contract AsyncPromise is AsyncPromiseStorage, Initializable, AddressResolverUtil
(bool success, , ) = localInvoker.tryCall(0, gasleft(), 0, combinedCalldata);

if (!success) {
// todo: in this case, promise will stay unresolved
revert PromiseRevertFailed();
}
}
Expand Down
27 changes: 12 additions & 15 deletions contracts/evmx/interfaces/IGasAccountManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,19 +41,6 @@ interface IGasAccountManager {

// ============ GAS ACCOUNT OPERATIONS ============

/// @notice Get available gas balance for an account
/// @dev Returns balance minus escrowed amount
function availableGas(address account) external view returns (uint256);

/// @notice Get total gas balance including escrowed
function totalGas(address account) external view returns (uint256);

/// @notice Get currently escrowed gas for an account
function escrowedGas(address account) external view returns (uint256);

/// @notice Approve an app to spend gas from your account
function approveGasSpending(address app, uint256 amount) external;

/// @notice Wrap native tokens into SGAS
function wrapToGas(address receiver) external payable;

Expand All @@ -64,7 +51,12 @@ interface IGasAccountManager {

/// @notice Deposit tokens from a chain into gas account
/// @dev Called by watcher after detecting GasStation deposit
function depositFromChain(bytes memory payload_) external;
function depositFromChain(
address token_,
address receiver_,
uint256 gasAmount_,
uint256 nativeAmount_
) external;

Comment on lines 52 to 60
Copy link

@coderabbitai coderabbitai bot Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

depositFromChain API is now explicit; ensure all implementors and callers are updated

Changing depositFromChain to (address token_, address receiver_, uint256 gasAmount_, uint256 nativeAmount_) is a good move away from opaque bytes payloads and should make cross‑chain deposits less error‑prone. This should be reflected in every contract implementing IGasAccountManager and every caller (e.g. GasStation, watchers) so there are no lingering uses of the old encoded payload form.

🏁 Script executed:

#!/bin/bash
# Ensure all implementations and uses of depositFromChain match the new signature
rg -n "depositFromChain" -C3

Length of output: 2676

Parameter name mismatch in GasAccountManager.depositFromChain

The interface declares the second parameter as receiver_, but the implementation in GasAccountManager.sol:108 names it depositTo_. This violates the interface contract. The implementation should rename depositTo_ to receiver_ to match the interface signature and maintain consistency with all callers (GasStation and tests).

🤖 Prompt for AI Agents 
In contracts/evmx/interfaces/IGasAccountManager.sol around lines 52-60 and
implementation in contracts/.../GasAccountManager.sol at line 108, the interface
declares the second parameter as receiver_ but the implementation uses
depositTo_; rename the implementation parameter depositTo_ to receiver_ to match
the interface signature, update all references inside the function body and any
NatSpec/comments to use receiver_, and run tests to ensure callers
(GasStation/tests) remain compatible.

/// @notice Withdraw SGAS to tokens on another chain
function withdrawToChain(
Expand All @@ -87,5 +79,10 @@ interface IGasAccountManager {

/// @notice Settle escrowed gas to transmitter
/// @dev Called when payload completes successfully
function settleGasPayment(bytes32 payloadId, address consumeFrom, address transmitter, uint256 amount) external;
function settleGasPayment(
bytes32 payloadId,
address consumeFrom,
address transmitter,
uint256 amount
) external;
}
16 changes: 11 additions & 5 deletions contracts/evmx/plugs/GasStation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ import "../../utils/RescueFundsLib.sol";
import {InvalidTokenAddress} from "../../utils/common/Errors.sol";
import "../interfaces/IGasAccountToken.sol";

interface IGasAccountManager {
function depositFromChain(
address token_,
address receiver_,
uint256 gasAmount_,
uint256 nativeAmount_
) external returns (bytes memory payloadId);
}

/// @title GasStation
/// @notice Contract for managing fees on a network
/// @dev The amount deposited here is locked and updated in the EVMx for an app gateway
Expand Down Expand Up @@ -68,19 +77,16 @@ contract GasStation is IGasStation, PlugBase, AccessControl {
if (!whitelistedTokens[token_]) revert TokenNotWhitelisted(token_);

// Encode deposit parameters: (chainSlug, token, receiver, gasAmount, nativeAmount)
bytes memory payload = abi.encode(
socket__.chainSlug(),
bytes memory payloadId = IGasAccountManager(address(socket__)).depositFromChain(
token_,
receiver_,
gasAmount_,
nativeAmount_
);
// todo: IGasAccountManager(socket__).depositFromChain(token_, receiver_, gasAmount_, nativeAmount_);

// Create trigger via Socket to get unique payloadId
bytes32 payloadId = socket__.sendPayload(payload);
token_.safeTransferFrom(msg.sender, address(this), gasAmount_ + nativeAmount_);
emit GasDeposited(token_, receiver_, gasAmount_, nativeAmount_, payloadId);
emit GasDeposited(token_, receiver_, gasAmount_, nativeAmount_, bytes32(payloadId));
}
Comment on lines +80 to 90
Copy link

@coderabbitai coderabbitai bot Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix the deposit call target and return handling.

Line 80: This should not cast socket__ (the Socket contract) to IGasAccountManager. Socket does not implement depositFromChain, so the very first deposit will revert with an unknown selector. Even if you swap in the real GasAccountManager address, the call still reverts because you decode a bytes memory payload while the current implementation returns no data. Restore the previous socket payload flow or invoke the actual GasAccountManager contract, and align the ABI so the callee either returns the payload id or the caller stops decoding it. Until then, deposits are dead.

🤖 Prompt for AI Agents 
In contracts/evmx/plugs/GasStation.sol around lines 80–90, the code incorrectly
casts socket__ to IGasAccountManager and attempts to decode a bytes return from
depositFromChain; fix by calling the real GasAccountManager contract (use the
stored gasAccountManager address/type) instead of socket__, and align the
ABI/handling: if GasAccountManager.depositFromChain does not return data, stop
assigning/decoding bytes memory payloadId and instead rely on the socket flow
that creates/returns the payloadId (or emit/derive the payloadId from the
correct call); alternatively, update the callee ABI to return the payload id
(bytes/bytes32) and handle that return consistently so the deposit no longer
reverts.


/// @notice Withdraws tokens
Expand Down
20 changes: 11 additions & 9 deletions contracts/protocol/Socket.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ contract Socket is SocketUtils {
if (executeParams_.callType != WRITE) revert InvalidCallType();

// check if the plug is connected
uint32 switchboardId = _verifyPlugSwitchboard(executeParams_.target);
address switchboardAddress = _verifyPlugSwitchboard(executeParams_.target);

// check if the message value is sufficient
if (msg.value < executeParams_.value + transmissionParams_.socketFees)
Expand All @@ -65,13 +65,18 @@ contract Socket is SocketUtils {
bytes32 payloadId = executeParams_.payloadId;

// verify the payload id
_verifyPayloadId(payloadId, switchboardId, chainSlug);
_verifyPayloadId(payloadId, switchboardAddress, chainSlug);

// validate the execution status
_validateExecutionStatus(payloadId);

// verify the digest
_verify(payloadId, switchboardId, executeParams_, transmissionParams_.transmitterProof);
_verify(
payloadId,
switchboardAddress,
executeParams_,
transmissionParams_.transmitterProof
);

// execute the payload
return _execute(payloadId, executeParams_, transmissionParams_);
Expand All @@ -88,11 +93,10 @@ contract Socket is SocketUtils {
*/
function _verify(
bytes32 payloadId_,
uint32,
address switchboardAddress,
ExecuteParams calldata executeParams_,
bytes calldata transmitterProof_
) internal {
address switchboardAddress = switchboardAddresses[switchboardId_];
// NOTE: the first un-trusted call in the system
address transmitter = ISwitchboard(switchboardAddress).getTransmitter(
msg.sender,
Expand Down Expand Up @@ -199,9 +203,7 @@ contract Socket is SocketUtils {
uint256 value_,
bytes calldata data_
) internal whenNotPaused returns (bytes32 payloadId) {
uint32 switchboardId = _verifyPlugSwitchboard(plug_);
address switchboardAddress = switchboardAddresses[switchboardId];

address switchboardAddress = _verifyPlugSwitchboard(plug_);
bytes memory plugOverrides = IPlug(plug_).overrides();

// Switchboard creates the payload ID and emits PayloadRequested event
Expand All @@ -212,7 +214,7 @@ contract Socket is SocketUtils {
);
}

/**
/**F
* @notice Fallback function that forwards all calls to Socket's sendPayload
* @dev The calldata is passed as-is to the switchboard
* @return The payload ID
Expand Down
14 changes: 7 additions & 7 deletions contracts/protocol/SocketUtils.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,25 +120,26 @@ abstract contract SocketUtils is SocketConfig {
/**
* @notice Verifies the plug switchboard and returns the switchboard id
* @param plug_ The address of the plug
* @return switchboardId The id of the switchboard
* @return switchboardAddress The address of the switchboard
*/
function _verifyPlugSwitchboard(
address plug_
) internal view returns (uint32 switchboardId) {
switchboardId = plugSwitchboardIds[plug_];
) internal view returns (address switchboardAddress) {
uint32 switchboardId = plugSwitchboardIds[plug_];
if (switchboardId == 0) revert PlugNotFound();
if (isValidSwitchboard[switchboardId] != SwitchboardStatus.REGISTERED)
revert InvalidSwitchboard();
switchboardAddress = switchboardAddresses[switchboardId];
}

function _verifyPayloadId(
bytes32 payloadId_,
uint32 switchboardId_,
address switchboardAddress_,
uint32 chainSlug_
) internal view {
(uint32 verificationChainSlug, uint32 verificationSwitchboardId) = getVerificationInfo(payloadId_);
if (verificationChainSlug != chainSlug_) revert InvalidVerificationChainSlug();
if (verificationSwitchboardId != uint32(switchboardId_))
if (switchboardAddresses[verificationSwitchboardId] != switchboardAddress_)
revert InvalidVerificationSwitchboardId();
}

Expand All @@ -148,8 +149,7 @@ abstract contract SocketUtils is SocketConfig {
* @param feesData_ Encoded fees data (type + data)
*/
function increaseFeesForPayload(bytes32 payloadId_, bytes calldata feesData_) external payable {
uint32 switchboardId = _verifyPlugSwitchboard(msg.sender);
address switchboardAddress = switchboardAddresses[switchboardId];
address switchboardAddress = _verifyPlugSwitchboard(msg.sender);
ISwitchboard(switchboardAddress).increaseFeesForPayload{value: msg.value}(
payloadId_,
msg.sender,
Expand Down
3 changes: 1 addition & 2 deletions contracts/protocol/switchboard/SwitchboardBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ abstract contract SwitchboardBase is ISwitchboard, AccessControl {
) external view returns (address transmitter) {
transmitter = transmitterSignature_.length > 0
? _recoverSigner(
// TODO: use api encode packed
keccak256(abi.encode(address(socket__), payloadId_)),
keccak256(abi.encodePacked(address(socket__), payloadId_)),
transmitterSignature_
)
: address(0);
Expand Down
1 change: 0 additions & 1 deletion contracts/utils/common/Structs.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ enum EscrowState {
struct EscrowEntry {
address account; // Who's paying
uint256 amount; // How much is escrowed
uint256 timestamp; // When escrowed
EscrowState state; // Current state
}

Expand Down
2 changes: 1 addition & 1 deletion script/helpers/TransferRemainingGas.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ contract TransferRemainingGas is Script {
address appGateway = vm.envAddress("APP_GATEWAY");
address newAppGateway = vm.envAddress("NEW_APP_GATEWAY");

uint256 totalGas = gasAccountManager.totalGas(appGateway);
uint256 totalGas = gasAccountToken.totalBalanceOf(appGateway);
uint256 payloadEscrow = totalGas - gasAccountToken.balanceOf(appGateway);
console.log("App Gateway:", appGateway);
console.log("New App Gateway:", newAppGateway);
Expand Down
Loading