Staking Contracts - Aria Protocol

The StakingTicket contract uses the standard ERC721 _mint() function instead of _safeMint() when creating new staking tickets in the mint() function:

_mint(_to, newTokenId);

Unlike _safeMint(), the _mint() function does not verify whether the recipient can handle ERC721 tokens when the recipient is a contract.

This implementation allows staking tickets to be minted to contract addresses that do not implement the IERC721Receiver.onERC721Received() function, potentially resulting in tickets being permanently locked and inaccessible. When this happens, the collateral represented by these tickets (RWIP tokens) would also be effectively lost.

Replace the instance of _mint() with _safeMint() in the contract implementation to ensure recipients can properly handle ERC721 tokens.

SOLVED: The Aria Protocol team solved this finding in commit e0854d6 by following the mentioned recommendation.

The RWIPStaking contract uses raw ecrecover for signature verification without protection against signature malleability. This allows multiple valid signatures to be created for the same message, potentially undermining security mechanisms that rely on signature uniqueness.

When verifying KYC signatures in the _checkKYCSignature function, the contract uses Ethereum's native ecrecover function:

address recoveredAddress = ecrecover(ethSignedMessageHash, v, r, s);
require(hasRole(KYC_SIGNER_ROLE, recoveredAddress), "Not Auth");

This implementation does not enforce signature canonicalization per EIP-2, which requires s values to be in the lower half of the curve. Due to properties of elliptic curve cryptography, for any valid signature (r, s, v), another valid signature exists with parameters (r, curve_order - s, flipped v), where flipped v toggles between 27 and 28.

This vulnerability could:

1. Undermine any future replay protection based on signature uniqueness.

2. Allow the same KYC attestation to be processed with different signature representations.

3. Cause inconsistencies in systems that track or index KYC attestations by signature.

While not immediately exploitable for fund theft, this vulnerability represents a deviation from best practices and could interact with other protocol features that assume signature uniqueness.

In the following scenario two different signatures can recover to same signer:

function test_signatureMalleability() public {
  // Setup: Define amount to stake
  uint256 amount = 100 ether;
  uint256 stakingHoldPeriod = 7 days;

  console.log("===> Setting up test for signature malleability");

  // First stake and redeem to get stakedRWIP tokens
  vm.startPrank(alice);
  rwipToken.approve(address(rwipStaking), amount);
  rwipStaking.stake(amount, stakingHoldPeriod);

  vm.warp(block.timestamp + stakingHoldPeriod + 1);
  rwipStaking.redeemTicket(0);
  vm.stopPrank();

  // Ensure the contract has RWIP tokens to return
  deal(address(rwipToken), address(rwipStaking), amount * 2);

  // Ensure alice does NOT have KYC status
  vm.prank(signerAdmin);
  rwipStaking.setKYCStatusForAddress(alice, false);

  // Create message hash for KYC attestation
  bytes32 messageHash = keccak256(abi.encodePacked("KYC ATTESTATION FOR: ", alice));
  bytes32 ethSignedMessageHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", messageHash));

  // Get original signature
  (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, ethSignedMessageHash);

  // Create malleated signature (with flipped s)
  // secp256k1 curve order n = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141
  bytes32 curveOrder = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141;
  bytes32 malleatedS = bytes32(uint256(curveOrder) - uint256(s));
  uint8 malleatedV = v == 27 ? 28 : 27; // Flip v between 27 and 28

  // Pack both signatures
  bytes memory originalSig = abi.encodePacked(r, s, v);
  bytes memory malleatedSig = abi.encodePacked(r, malleatedS, malleatedV);

  // Log the complete signatures (in hex)
  console.log("Original signature:");
  console.logBytes(originalSig);
  console.log("Malleated signature:");
  console.logBytes(malleatedSig);
  console.log("The signatures above are different but validate to the same signer");

  // Verify both signatures recover to the same address
  address recovered1 = ecrecover(ethSignedMessageHash, v, r, s);
  address recovered2 = ecrecover(ethSignedMessageHash, malleatedV, r, malleatedS);
  console.log("Both signatures recover to:", recovered1);
  assert(recovered1 == recovered2);
  assert(keccak256(originalSig) != keccak256(malleatedSig)); // Confirm signatures are different

  // Use both signatures to unstake in two separate transactions
  uint256 unstakeAmount = amount / 2;

  // First unstake with original signature
  vm.startPrank(alice);
  stakedRwip.approve(address(rwipStaking), unstakeAmount);
  rwipStaking.unstake(unstakeAmount, originalSig);

  // Second unstake with malleated signature
  stakedRwip.approve(address(rwipStaking), unstakeAmount);
  rwipStaking.unstake(unstakeAmount, malleatedSig);
  vm.stopPrank();
}

Replace the current signature verification with OpenZeppelin's ECDSA library, which includes malleability protection:

address recoveredAddress = ECDSA.recover(ethSignedMessageHash, _signature);

SOLVED: The Aria Protocol team solved this finding in commit 74ca8dd by following the mentioned recommendation.

The stake() function in the RWIPStaking contract does not validate that the staking amount is greater than zero, allowing users to mint unlimited staking tickets without actually staking any meaningful collateral.

The stake() function in the RWIPStaking contract lacks validation to ensure that _amount is greater than zero:

function stake(uint256 _amount, uint256 _stakingHoldPeriod) external {
    if (_stakingHoldPeriod < minStakingHoldPeriod) revert StakingHoldPeriodTooLow();

    rwipToken.safeTransferFrom(msg.sender, address(stakingTicket), _amount);
    uint256 stakingTicketId = stakingTicket.mint(msg.sender, _amount, _stakingHoldPeriod);
    emit RWIPStaked(msg.sender, stakingTicketId, _amount, _stakingHoldPeriod);
}

This allows users to call stake(0, validHoldPeriod) repeatedly, which:

1. Transfers 0 RWIP tokens (which succeeds, as ERC20 allows zero-amount transfers).

2. Mints a new staking ticket with 0 collateral.

3. Results in a valid ERC721 token being minted to the user.

While these tickets would each redeem for 0 stRWIP tokens, they remain valid NFTs that could potentially be leveraged for unintended purposes, potentially manipulating systems that use ticket token quantities for calculations.

In the following scenario, the attacker can mint 100 NFT tokens without having to transfer any ERC20 tokens to the contract:

function test_unlimitedMintWithZeroAmount() public {
  vm.startPrank(attacker);
  for (uint256 i = 0; i < 100; i++) rwipStaking.stake(0, 1 days);
  vm.stopPrank();
  assertEq(stakingTicket.balanceOf(attacker), 100);
}

Add validation to ensure that staking amounts are greater than zero.

SOLVED: The Aria Protocol team solved this finding in commit 612586a by following the mentioned recommendation.

Throughout the codebase, there are several instances where input values are assigned without proper validation. For example, ensuring that an input address is not the zero address.

Failing to validate inputs before assigning them to state variables can lead to unexpected system behavior or even complete failure.

Instances of this issue include:

• In StakingTicket.initialize(), _rwipToken and _stakingContract are not checked against the zero address before assignment.

  
  

• In StakingTicket.setRWIPToken(), _rwipToken is not validated to ensure it's not the zero address before assignment.

  
  

• In StakingTicket.setStakingContract(), _stakingContract is not validated to ensure it's not the zero address before assignment.

  
  

• In RWIPStaking.initialize(), _rwipToken, _stakedRWIPToken, _stakingTicket, and _kycSignerAdmin are not checked against the zero address before assignment, and the _minStakingHoldPeriod is not verified to fall within a valid threshold.

  
  

• In RWIPStaking.unstake(), there's no validation to ensure that _amount is greater than zero. This allows users to unstake 0 tokens, which would waste gas and emit misleading events with zero values.

  
  

• In RWIPStaking.setRWIPToken(), _rwipToken is not checked against the zero address before assignment.

  
  

• In RWIPStaking.setMinStakingHoldPeriod(), _minStakingHoldPeriod is not verified to fall within a valid threshold.

  
  

• In RWIPStaking.setStakedRWIPToken(), _stakedRWIPToken is not checked against the zero address before assignment.

  
  

• In RWIPStaking.setStakingTicket(), _stakingTicket is not checked against the zero address before assignment.

Add proper validation to ensure that the input values are within expected ranges and that addresses are not the zero address. This will help prevent unexpected behavior and improve the overall robustness of the code.

ACKNOWLEDGED: The Aria Protocol team made a business decision to acknowledge this finding and not alter the contracts.

Throughout the contracts in scope, there are several instances where administrative functions change contract state by modifying core state variables without them being reflected in event emissions. The absence of events may hamper effective state tracking in off-chain monitoring systems.

Instances of this issue can be found in:

• StakingTicket.setRWIPToken()

• StakingTicket.setStakingContract()

• RWIPStaking.withdraw()

• RWIPStaking.setKYCStatusForAddress()

• RWIPStaking.setMinStakingHoldPeriod()

• RWIPStaking.setRWIPToken()

• RWIPStaking.setStakedRWIPToken()

• RWIPStaking.setStakingTicket()

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

PARTIALLY SOLVED: The Aria Protocol team partially solved this finding in commit bfe98bdbadcbfb33b6e4affcbe9b191bb9f9a382 by following the mentioned recommendation and adding events to some of the aforementioned functions.

In the StakedRWIP contract, there is a typo, where the function initialize() is misspelled as intialize().

This typo may affect the functionality of the code and initialization of the contract, depending on the framework used for deployment.

It is recommended to fix the typo to improve the readability of the codebase and facilitate proper initialization.

SOLVED: The Aria Protocol team solved this finding in commit 0c53aaf by following the mentioned recommendation.

Throughout the files in scope, there are several instances where revert strings are used over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

or starting from Solidity 0.8.27 :

require(condition, ConditionNotMet());

For more references, see here and here.

SOLVED: The Aria Protocol team solved this finding in commit 4db3df6 by following the mentioned recommendation.

The addressHasKYC mapping in unnamed despite using a Solidity version that supports named mappings.

Named mappings improve code readability and self-documentation by explicitly stating their purpose.

Consider refactoring the mapping to use named arguments, which will enhance code readability and make the purpose of the mapping more explicit.

ACKNOWLEDGED: The Aria Protocol team made a business decision to acknowledge this finding and not alter the contracts.