Organization
- @centrifuge
Engagement Type
Cantina Reviews
Period
-
Repositories
Researchers
Findings
Medium Risk
3 findings
3 fixed
0 acknowledged
Low Risk
5 findings
3 fixed
2 acknowledged
Informational
4 findings
3 fixed
1 acknowledged
Medium Risk3 findings
AxelarAdapter.execute will fail to process any incoming message
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Drastic Watermelon
Summary
AxelarAdapter.execute
will fail to process any incoming message becauseCastLib.toAddress(string)
will always revert for any input provided by Axelar's infrastructure.Finding Description
CastLib.toAddress(string)
incorrectly expects the input string's length to be equal to 20:function toAddress(string calldata addr) internal pure returns (address) { require(bytes(addr).length == 20, "Input should be 20 bytes"); return address(bytes20(bytes(addr)));}
When relaying a cross chain transaction to the destination chain, Axelar provides the cross chain transaction's sender's address as a string, including a leading "0x": this may be verified by inspecting the
sourceAddress
calldata parameter of the destination chain's side of an Axelar cross chain transaction: example transaction.Because all of the above, the
require
statement's condition will always evaluate tofalse
:Welcome to Chisel! Type `!help` to show available commands.➜ string memory str = "0xe6B3949F9bBF168f4E3EFc82bc8FD849868CC6d8"Traces: [211] 0xBd770416a3345F91E4B34576cb804a576fa48EB1::run() └─ ← [Stop] ➜ bytes(str).length == 20Type: bool└ Value: false ➜ bytes(str).length == 42Type: bool└ Value: true
Impact Explanation
AxelarAdapter.execute
failing for any incoming message, implies the inability for it to reach quorum within the Gateway contract. This in turn implies that no cross chain message will ever be processed by the system.Likelihood Explanation
The issue will manifest for any cross chain message relayed with Axelar.
Recommendation
CastLib.toAddress(string)
should verify the conditionbytes(addr).length == 42
.AxelarAdapter.send sets incorrect destinationAddress for every cross chain transaction
Severity
- Severity: Medium
Submitted by
Drastic Watermelon
Summary
Finding Description
Axelar cross chain transactions may be initiated by using
IGateway.callContract
. In this call, senders must provide the contract intended to receive a cross chain contract call in thecontractAddress
parameter.AxelarAdapter.send
usesCastLib.toString
to encode an address as a string, providing this method's output as the recipient of Axelar's cross chain contract call:function toString(address addr) internal pure returns (string memory) { return string(abi.encodePacked(addr));}
Because the
CastLib.toString
method formats an address as a string by returningstring(abi.encodePacked(addr))
it returns the UTF-8 encoded string represented byaddr
's raw hex bytes:Welcome to Chisel! Type `!help` to show available commands.➜ address addy = 0xe6B3949F9bBF168f4E3EFc82bc8FD849868CC6d8;Traces: [133] 0xBd770416a3345F91E4B34576cb804a576fa48EB1::run() └─ ← [Stop] ➜ string(abi.encodePacked(addy))Type: string├ UTF-8: 泔����N>�����I����├ Hex (Memory):├─ Length ([0x00:0x20]): 0x000000000000000000000000000000000000000000000000000000000000002e├─ Contents ([0x20:..]): 0xe6b394efbfbdefbfbdefbfbd16efbfbd4e3eefbfbdefbfbdefbfbdefbfbdefbfbd49efbfbdefbfbdefbfbdefbfbd000000000000000000000000000000000000├ Hex (Tuple Encoded):├─ Pointer ([0x00:0x20]): 0x0000000000000000000000000000000000000000000000000000000000000020├─ Length ([0x20:0x40]): 0x000000000000000000000000000000000000000000000000000000000000002e└─ Contents ([0x40:..]): 0xe6b394efbfbdefbfbdefbfbd16efbfbd4e3eefbfbdefbfbdefbfbdefbfbdefbfbd49efbfbdefbfbdefbfbdefbfbd000000000000000000000000000000000000
Impact Explanation
Axelar cross chain contract calls will be sent to incorrect recipients.
Likelihood Explanation
The highlighted issue will manifest for any cross chain contract call sent via AxelarAdapter.
Recommendation
To correcty encode an address as a string according to how Axelar expects,
CastLib.toString
should use or emulate OpenZeppelin's Strings library.Edge case in request handling can cause deposits/redemptions to be broken
Severity
- Severity: Medium
Submitted by
Liam Eastwood
Description
Due to the asynchronous nature of Centrifuge V3, pool request cancellations following the initial request to deposit/redeem may be handled in-order on the destination chain but out-of-order on the source chain. This can cause permanent locking of future deposit/redemption requests, leading to a liveness issue for users.
The exact scenario for when deposits are potentially locked for an unknown amount of time is outlined below:
- A deposit is made from a non-pool chain.
- Before the deposit request has been relayed to the pool chain and updated
userOrder.pending
, the same user request to cancel the same deposit request. - The cancel deposit request is handled first by calling
Hub.cancelDepositRequest()
and becausecancelledAssetAmount = 0
, no message is relayed back to the non-pool chain. - Subsequently, the deposit request is handled on the pool chain and correctly fulfilled as expected.
- But due to the rounding mentioned above,
userOrder.pending
is not fully zeroed out and may never be fully handled as long as there are other requests from users in a given epoch. - Consequently, future deposits will not function as expected because
AsyncRequestManager.requestDeposit()
expectsstate.pendingCancelDepositRequest != true
which will never be possible because of point 5.
It is important to note that the more severe case for this issue is in redemptions, hence the outlined scenario mentioned above would also apply here.
Recommendation
Some considerations need to be made here to adjust any rounded balances in the
ShareClassManager
contract when claiming deposits/redemptions to ensure leftover amounts can always be cancelled to unlock new requests on the source chain.
Low Risk5 findings
BytesLib.sliceZeroPadded adds non-zero bytes in padding
Severity
- Severity: Low
Submitted by
Drastic Watermelon
Description
BytesLib.sliceZeroPadded
can add non-zero bytes of padding.The method used has been forked from solidity-bytes-utils/BytesLib.sol, relaxing the condition at L238. While the intention for this change is to allow users of the library's method to request a
bytes
slice which ends after the initial data's end, the original implementation was not designed for this use case.Given that the highlighted method is not used in any crucial section of the codebase, no real impact is implied.
Proof of Concept
// SPDX-License-Identifier: BUSL-1.1pragma solidity 0.8.28; import "forge-std/Test.sol";import {BytesLib} from "src/misc/libraries/BytesLib.sol"; contract BytesLibTest is Test { function testSliceZeroPadded(bytes memory data, bytes memory randomStart, uint8 randomLength) public pure { bytes memory value = bytes.concat(randomStart, abi.encodePacked(data)); bytes memory expected; if (randomLength > data.length) { // Padding case bytes memory padding = new bytes(randomLength - data.length); expected = abi.encodePacked(data, padding); } else { // Base case, equivalent to original code's -- not really relevant for this test expected = BytesLib.slice(value, randomStart.length, randomLength); } bytes memory got = BytesLib.sliceZeroPadded(value, randomStart.length, randomLength); assertEq(got.length, expected.length, "!length"); assertEq( got, expected, "!bytes" ); } function testSliceZeroPadded_counterexample() public pure { bytes memory data = hex"deadbeef"; bytes memory start = hex"0000000000000000000000004e59b44847b379578588920ca78fbf26c0b4956c"; uint8 length = 38; testSliceZeroPadded(data, start, length); // fails with "!bytes" }}
Recommendation
The method ought to be rewritten in Solidity to improve its clarity and readability. For example, the method could leverage the original implementation to obtain the initial data's slice and then add padding:
function sliceZeroPadded_solidity(bytes memory _bytes, uint256 _start, uint256 _length) external pure returns (bytes memory) { bool needsPad = _bytes.length < _start + _length; if (!needsPad) return slice(_bytes, _start, _length); bytes memory temp = slice(_bytes, _start, _bytes.length - _start); return abi.encodePacked(temp, new bytes(_length + _start - _bytes.length));}
ERC20.burn requires wards to have been granted an allowance to burn tokens
State
- Acknowledged
Severity
- Severity: Low
Submitted by
Drastic Watermelon
Description
ERC20.burn
is decorated with theauth
modifier, implying the method is only callable by authorized wards.At the same time, if method's caller is different from the account whose tokens are being burnt, the method will attempt to consume the allowance granted by the token holder to the caller.
As a consequence, wards can be denied the ability of burning user tokens in case no allowance is ever set.
Recommendation
Either remove the
auth
modifier or avoid consuming the allowance.VaultRouter.enableLockDepositRequest can revert when wrapping underlying tokens
Severity
- Severity: Low
Submitted by
Drastic Watermelon
Description
VaultRouter.enableLockDepositRequest
, whenvaultDetails.isWrapper && assetBalance < amount
holds, attempts to wrap tokens on the user's behalf before incrementing a user's locked request.When wrapping tokens via
VaultRouter.wrap
, the contract will wrap the minimum between the requestamount
and the user's balance ofunderlying
tokens:function wrap(address wrapper, uint256 amount, address receiver, address owner) public payable protected { require(owner == msg.sender || owner == address(this), InvalidOwner()); address underlying = IERC20Wrapper(wrapper).underlying(); amount = MathLib.min(amount, IERC20(underlying).balanceOf(owner)); require(amount != 0, ZeroBalance()); SafeTransferLib.safeTransferFrom(underlying, owner, address(this), amount); _approveMax(underlying, wrapper); require(IERC20Wrapper(wrapper).depositFor(receiver, amount), WrapFailed());}
In case that
IERC20(underlying).balanceOf(owner) < amount
holds and less tokens thanamount
are wrapped,VaultRouter.enableLockDepositRequest
will still useamount
when invokingVaultRouter.lockDepositRequest
, eventually leading it to attempt to pull more funds than held byowner
, making the transaction revert.Recommendation
VaultRouter.wrap
should return the amount of wrapped tokens obtained, which should be passed toVaultRouter.lockDepositRequest
.Holding amounts may be updated erroneously when queueing asset amounts
State
- Acknowledged
Severity
- Severity: Low
Submitted by
Liam Eastwood
Description
The
BalanceSheet
contract facilitates deposits and withdrawals by pushing/pulling assets to and from the escrow. Share token burns and mints are being handled here too, each of which have direct influence on thetotalIssuance
being tracked and the asset holding balances.When the
AsyncRequestManager
approves deposits, it notes the asset amount in theBalanceSheet
. If the queue is enabled, thesender.sendUpdateHoldingAmount()
call is delayed until the manager callsBalanceSheet.submitQueuedAssets()
.It becomes problematic when the pool manager calls
PoolManager.updatePricePoolPerShare()
while there are existing assets queued. Consequently, the later call toBalanceSheet.submitQueuedAssets()
will update holding amounts on the latest/currentpricePoolPerAsset
which is not entirely accurate.Ultimately, the
Holdings
contract is exposed to fluctuations in the asset <-> pool currency exchange rate that happens while asset amounts are being queued.Recommendation
It does seem that the Hub manager can make adjustments to the holding amounts by calling
Hub.updateHoldingValue()
so it might not be worth making a direct fix to this.Non-batched Hub transactions may fail to relay payload data to adapters
Severity
- Severity: Low
Submitted by
Liam Eastwood
Description
There are two instances where a non-batched transaction will partially fail at the
Gateway
because the transaction is refunded after the firstGateway.send()
call.Hub.notifyDeposit()
Hub.notifyRedeem()
These aforementioned functions have an internal
_pay()
function which attempts to subsidize payload sending costs to the adapters but will subsequently refund the leftover amount after the first send to theGateway
contract, causing the second payload send to be underpaid. This leads to lacklustre user experience as users are expected to repay the underpaid batch.Recommendation
Modify affected functions in the
Hub
contract to handle this correctly.
Informational4 findings
BytesLib.sliceZeroPadded reverts with panic instead of custom error
Severity
- Severity: Informational
Submitted by
Drastic Watermelon
Description
BytesLib.sliceZeroPadded
executes an overflow check by ensuring that_length + 31 >= _length
holds.Because the check is executed in checked arithmetic, when
_length + 31
does overflow, execution will revert with a panic error instead of the customSliceOverflow
error.Recommendation
Wrap the
require
check in anunchecked
block.System assumes ERC6909 tokens cannot have tokenId = 0
State
- Acknowledged
Severity
- Severity: Informational
Submitted by
Drastic Watermelon
Description
Different contracts implement a unified interface to interact with ERC-20 and ERC-6909 tokens: when
tokenId == 0
is provided, the system assumes it must interact with an ERC-20 token.Because the ERC-6909 spec doesn't specify any restriction for the
tokenId
field, the system may face compatibility issues with future ERC-6909 implementations being used.Note that Uniswap's ERC-6909 implementation is unaffected, as
tokenId
s are assigned as an ERC-20's address cast touint160
.Recommendation
Refactor the system to also support using ERC-6909 tokens with
tokenId = 0
.Gateway contract deployment fails when deployer != msg.sender
Severity
- Severity: Informational
Submitted by
Liam Eastwood
Description
The
Gateway
contract's constructor makes an internal call tosetRefundAddress(GLOBAL_POT, IRecoverable(address(this)))
which has anauth
modifier. The initial authentication on deployment is only ever granted to thedeployer
parameter in the constructor and hence this internal call will fail ifdeployer != msg.sender
.Recommendation
Consider documenting whether or not this is intended and making adjustments where necessary to accommodate more permissive behaviour.
Gateway repay and retry actions do not adhere to "Checks-Effects-Interactions" guidelines
Severity
- Severity: Informational
Submitted by
Liam Eastwood
Description
Typical implemented behaviour for a function which makes any external call is to update all storage if possible before the call is made to avoid complex forms of reentrancy. It's unclear if there is any attack vector here because the
underpaid
andfailedMessages
counters are eventually updated and to circumvent this, an attacker would need to takeover theGateway
contract.Recommendation
Consider updating the
repay()
andretry()
functions to be CEI compatible.