Ondo Finance - adriro's results

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L121-L129 https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L234-L240

Vulnerability details

Chain support cannot be removed or cleared in bridge contracts

Due to how addresses are handled and stored in the configuration settings, it is not possible to remove chain support in both source and destination bridge contracts.

Impact

The Axelar bridge service uses strings to represent addresses: messages sent to another chain need to specify its destination contract as a string. The protocol decided to follow the same representation and contract store addresses as strings as part of their configuration.

Chain support in the bridge contracts is then represented by associating the chain name with the address of the contract, stored as a string. This can be seen in the implementation of setDestinationChainContractAddress() for the SourceBridge contract, which stores the string, and the implementation of addChainSupport() in the DestinationBridge contract, which stores the hash of the string:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L121-L129

121:   function setDestinationChainContractAddress(
122:     string memory destinationChain,
123:     address contractAddress
124:   ) external onlyOwner {
125:     destChainToContractAddr[destinationChain] = AddressToString.toString(
126:       contractAddress
127:     );
128:     emit DestinationChainContractAddressSet(destinationChain, contractAddress);
129:   }

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L234-L240

234:   function addChainSupport(
235:     string calldata srcChain,
236:     string calldata srcContractAddress
237:   ) external onlyOwner {
238:     chainToApprovedSender[srcChain] = keccak256(abi.encode(srcContractAddress));
239:     emit ChainIdSupported(srcChain, srcContractAddress);
240:   }

This also means that checks need to be done based on the stored representation. SourceBridge checks the length of the string, while DestinationBridge checks for a bytes32(0) value:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L68-L71

68:     if (bytes(destContract).length == 0) {
69:       revert DestinationNotSupported();
70:     }
71: 

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L96-L98

96:     if (chainToApprovedSender[srcChain] == bytes32(0)) {
97:       revert ChainNotSupported();
98:     }

Note that this implies that there is no way to clear these settings and remove chain support. In the case of SourceBridge, any address sent to setDestinationChainContractAddress() will be converted to their string representation which will always have length greater than zero. For the DestinationBridge, addChainSupport() will hash the address parameter and it will be impossible for that hash value to be zero (since it will imply knowing the preimage of zero).

Proof of Concept

1. Admin configs destination address in SourceBridge by calling setDestinationChainContractAddress("optimism", destinationAddress).
2. Admin decides to remove support for Optimism.
3. Admin calls setDestinationChainContractAddress("optimism", address(0)), but this will actually store the string for the zero address "0x0000....000".
4. The check bytes(destContract).length == 0 will fail and messages will still be routed.

Recommended Mitigation Steps

Provide functions in both contracts to allow the owner to clear the settings by resetting their configuration to the default value.

function removeDestinationChainContractAddress(
  string memory destinationChain
) external onlyOwner {
  delete destChainToContractAddr[destinationChain];
  emit DestinationChainContractAddressRemoved(destinationChain);
}

function removeChainSupport(
  string calldata srcChain
) external onlyOwner {
  delete chainToApprovedSender[srcChain];
  emit ChainIdRemoved(srcChain);
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L109

Vulnerability details

Bridged messages containing the same information coming from different source chains may clash in the destination bridge and lead to loss of tokens.

Impact

Tokens are bridged by encoding some information that is sent as the payload of the bridged message through the Axelar service. The destination bridge then recovers this information and uses its hash as the key value to identify the transaction while it is under review by the approvers.

The SourceBridge contract encodes the current version, the sender, amount and current nonce as the payload of the message:

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/SourceBridge.sol#L79-L81

79:     bytes memory payload = abi.encode(VERSION, msg.sender, amount, nonce++);
80: 
81:     _payGasAndCallContract(destinationChain, destContract, payload);

The DestinationBridge receives this payload and hashes it using keccak256 that serves as the key of both mappings txnHashToTransaction (which holds the transaction information) and txnToThresholdSet (which holds the current state of approvals):

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L108-L109

108:     bytes32 txnHash = keccak256(payload);
109:     txnHashToTransaction[txnHash] = Transaction(srcSender, amt);

Note that the payload doesn't contain the source bridge address, and the mapping isn't scoped to the source chain, the key is just the hash of the payload. If an account wants to bridge the same amount of tokens from different source chains, which also happen to have by coincidence the same current nonce in their source bridge, both messages will end up having the same payload and the same value for their hash, causing a clash in these mappings.

Since it is likely that bridged messages will need confirmation by approvers (i.e. minting won't happen automatically in _execute()), the described will lead to loss of funds for the user: while the first transaction is pending to be executed as it is being reviewed by the approvers, the next transaction will overwrite all the data from the first, leading to information loss and preventing the minting of tokens from the first transaction that arrived.

Proof of Concept

User has the same amount of tokens in both source chain 1 (SC1) and source chain 2 (SC2) and wants to bridge them to the destination chain (DC). State of source bridge in both SC1 and SC2 currently contains the same nonce value.

1. User calls burnAndCallAxelar(amount, DC) in SC1.
2. DC receives message through execute(). This will hash the payload as txnHash and store this transaction in txnHashToTransaction[txnHash] while it is under review to be approved.
3. User calls burnAndCallAxelar(amount, DC) in SC2.
4. Again, DC receives message through execute() and hashes the payload. Since the information is the same (same version, same sender, same amount and same nonce) the hash happens to be the same as before, and will end up overriding the values in the txnHashToTransaction mapping as the key is the same.
5. The first transaction will be lost as all of the information will be overwritten.

Recommended

Scope the txnHashToTransaction mapping to include the address of the source bridge, or alternatively include the source address as part of the bridged payload.

Assessed type

Other

Report

Summary

Low Issues

Total of 10 issues:

Non Critical Issues

Total of 2 issues:

Low Issues

<a name="L-1"></a>[L-1] Confusing semantics for bridged messages approvals

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L111

Bridged messages get an "automatic" approval while the message is being executed in the destination bridge contract, which means that all messages effectively get one approval as soon as they are bridged.

This implies that "real" approvals would require setting a threshold with a numberOfApprovalsNeeded of at least two.

This brings a lot of unnecessary confusing semantics to the approvals scheme and could potentially lead to errors in configuration settings that would allow messages to go through automatically when actually they are expected to be approved.

It is recommended to remove this automatic approval and treat each approval as an explicit approval action of the set of enabled approvers.

<a name="L-2"></a>[L-2] No default threshold configuration

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L128

The implementation of _attachThreshold() loops through each of the configured set of thresholds searching for the element that adjusts to the bridged amount.

If no configuration is found the function will revert, the message will be lost and it will require manual handling.

It is recommended setting a default value (a high value number of approvals) to be used when no threshold configuration matches.

<a name="L-3"></a>[L-3] Validate number of approvers in setThresholds()

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L255

The setThresholds() function should validate that elements in the numOfApprovers array are greater than zero.

<a name="L-4"></a>[L-4] Missing safe wrapper for ERC20 transfer in rescueTokens()

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L322

Use a "safe" wrapper to execute ERC20 transfer for better compatibility.

<a name="L-5"></a>[L-5] Account may get blacklisted during the bridge process

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L349

Accounts that bridge tokens may get blacklisted in the source chain after the bridging process had been initiated. This could lead to different issues:

• If the account is not blacklisted in the destination chain, the user may still operate their funds.
• If the account is blacklisted, then the function will revert and could be treated as a failure that would require manual intervention.

<a name="L-6"></a>[L-6] Missing validation for index parameter in overrideRange()

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L186

The overrideRange() function should validate that indexToModify is within bounds, i.e. require(indexToModify < rangeLength).

<a name="L-7"></a>[L-7] Oracle assumes asset has 18 decimals

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L282

The roundUpTo8() implementation assumes the asset has 18 decimals as the rounding is done using 1e10 (since 18 - 8 = 10). It is recommended to use a constant to properly state this dependency and for better understanding.

<a name="L-8"></a>[L-8] Wrong argument in Transfer event of wrap() function

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L438

The third argument to the Transfer event is expecting the amount of rUSDY tokens but it is wrongly called with getRUSDYByShares(_USDYAmount), which lacks the proper scaling to convert the USDY amount to shares.

The correct version should be:

emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount * BPS_DENOMINATOR));

<a name="L-9"></a>[L-9] Wrong argument in TransferShares event of wrap() function

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L439

The third argument to the TransferShares event is expecting the amount of shares but it is wrongly called with _USDYAmount, which is the amount of USDY tokens, not the shares.

The correct version should be:

emit TransferShares(address(0), msg.sender, _USDYAmount * BPS_DENOMINATOR);

<a name="L-10"></a>[L-10] Contracts can be re-deployed in rUSDY factory

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L75

The rUSDY contract can be redeployed by the guardian by calling deployrUSDY() in the rUSDYFactory contract. This will deploy a new set of contracts and overwrite the storage variables that link to the contract.

It is recommended to add a check to avoid redeployment once contracts have been deployed.

Non Critical Issues

<a name="NC-1"></a>[NC-1] Missing calls to base initializers in rUSDY

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L120

The __rUSDY_init() function doesn't call the initializers for some of the base contracts:

• Initializable
• ContextUpgradeable
• PausableUpgradeable
• AccessControlEnumerableUpgradeable

<a name="NC-2"></a>[NC-2] Missing event to notify oracle has changed in rUSDY

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L662

The setOracle() function should emit an event to signal off-chain actors that the oracle has been updated.

DestinationBridge contract

• Store value of chainToApprovedSender[srcChain] as a local variable instead of reading it multiple times from storage. The same value is read 4 times:
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L96
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L99
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L102
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L106

• Validation in line 96 (chainToApprovedSender[srcChain] == bytes32(0)) is not needed as it is already covered by the validation in line 99 (chainToApprovedSender[srcChain] != keccak256(abi.encode(srcAddr))).
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L96

• Avoid copying the full array of Treshold struct to memory while reading chainToThresholds[srcChain] in _attachThreshold() function. Alias the variable to storage instead.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L135

• Improve storage of approvers by using a mapping instead of an array. Currently the array of approvers needs to traverse each time an approver grants approval to check for duplicate entries. By using a mapping, the same operation can be done in O(1).
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L160-L164

• Avoid copying the TxnThreshold struct to memory while reading txnToThresholdSet[txnHash] in _checkThresholdMet() function. Alias the variable to storage instead.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L178

• The check in line 270 can use the local amounts array instead of fetching the entry from storage.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L270

• Both branches of the if in line 265 have the same duplicated code that push the entry to the chainToThresholds[srcChain]. The code can be refactored to execute the operation outside the if statement and avoid code duplication.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L265

• In rescueTokens(), transfer the tokens to msg.sender instead of reading the owner from storage.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L324

• Avoid copying the Transaction struct to memory while reading txnHashToTransaction[txnHash] in _mintIfThresholdMet() function. Alias the variable to storage instead.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/bridge/DestinationBridge.sol#L339

RWADynamicOracle contract

• In getPrice() loop using the indices in revese order (i = length - 1; i >=0l; --i) instead of calculating the (length - 1) - i expression in every iteration.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L75

• In getPrice(), the range.end variable is read from storage two times. Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L80
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L81

• In setRange(), the length of the ranges array is fetched from storage two times. Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L155
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L165

• Avoid copying the Range struct to memory while reading currentRange in derivePrice() function. Alias the variable to storage instead.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L263

• Reduce size of variables in Range struct to enable efficient storage packing.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L295-L300

• The _mul doesn't need to check for overflow since checked arithmetic is already provided in Solidity 0.8
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/rwaOracles/RWADynamicOracle.sol#L404-L406

rUSDY contract

• Consider not updating allowance if it the current allowance is set to an "infinite" (type(uint256).max) value.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L310

• Calculation in line 310 can be unchecked since the condition has been already checked in line 307.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L310

• Calculation in line 362 can be unchecked since the condition has been already checked in line 359.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L362

• The wrap() function doesn't require the whenNotPaused since the implementation of _mintShares already checks this.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L434

• The unwrap() function doesn't require the whenNotPaused since the implementation of _burnShares already checks this.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L449

• Calculation in line 530 can be unchecked since the condition has been already checked in line 526.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L530

• Calculation in line 531 can be unchecked since overflow is already covered by the totalShares variable.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L531

• The totalShares variable is re-fetched from storage in _mintShares. Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L555

• Calculation in line 590 can be unchecked since the condition has been already checked in line 584.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L590

• The totalShares variable is re-fetched from storage in _burnShares. Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L601

rUSDYFactory contract

• The rUSDYImplementation is read multiple times from storage in deployrUSDY(). Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L82
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L85
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L104
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L111

• The rUSDYProxyAdmin is read multiple times from storage in deployrUSDY(). Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L83
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L86
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L99
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L100
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L103
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L110

• The rUSDYProxy is read multiple times from storage in deployrUSDY(). Consider using a local variable cache.
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L84
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L89
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L102
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L109

• Remove unnecessary assert() in deployrUSDY()
  https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDYFactory.sol#L100