Platform: Code4rena
Start Date: 01/09/2023
Pot Size: $36,500 USDC
Total HM: 4
Participants: 70
Period: 6 days
Judge: kirk-baird
Id: 281
League: ETH
Rank: 31/70
Findings: 3
Award: $116.18
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: adriro
Also found by: 0x6980, 0xStalin, 0xanmol, 0xmystery, 0xpanicError, Arz, Aymen0909, BenRai, Breeje, Lalanda, MohammedRizwan, Raihan, SovaSlava, Stormreckson, Udsen, ast3ros, bin2chen, castle_chain, catellatech, codegpt, dev0cloo, gkrastenov, hals, klau5, kutugu, ladboy233, matrix_0wl, nirlin, ohm, peanuts, pipidu83, sandy, wahedtalash77
87.5807 USDC - $87.58
rescueTokens()
function should prevent the withdrawal of USDY tokens .the function rescueTokens()
should make sure that the token to be withdrawn is not the core token of the bridge , but any tokens that had been sent by mistake , consider add this check :
function rescueTokens(address _token) external onlyOwner { + if(_token == address(TOKEN)) revert(); uint256 balance = IRWALike(_token).balanceOf(address(this)); IRWALike(_token).transfer(owner(), balance); }
in the function setThresholds()
there is no check if the numberOfApprovers
is equal to 0 which be able to prevent receiving a messages of this amount .
consider add a check to make sure that the number of approvers in greater than zero .
for (uint256 i = 0; i < amounts.length; ++i) { + if (numOfApprovers[i] == 0 ) revert(); if (i == 0) { chainToThresholds[srcChain].push( Threshold(amounts[i], numOfApprovers[i]) ); } else { if (chainToThresholds[srcChain][i - 1].amount > amounts[i]) { revert ThresholdsNotInAscendingOrder(); } chainToThresholds[srcChain].push( Threshold(amounts[i], numOfApprovers[i]) ); } }
chainToApprovedSender
to allow the owner to deactivate receiving messages from a specific chain .there is only the method to add chain and there is no removing mechanism code snippet
the function approve()
allow approving a deleted transaction, and if the number reach the threshold the function will mint zero tokens to the zero address , so it is better to prevent this from happen .
should make sure that if(txn.sender == address(0)) revert();
function approve(bytes32 txnHash) external { if (!approvers[msg.sender]) { revert NotApprover(); } _approve(txnHash); _mintIfThresholdMet(txnHash); }
first need to make the function getNumApproved() public function , then
function _checkThresholdMet(bytes32 txnHash) internal view returns (bool) { - TxnThreshold memory t = txnToThresholdSet[txnHash]; - if (t.numberOfApprovalsNeeded <= t.approvers.length) { + if (t.numberOfApprovalsNeeded <= getNumApproved(txnHash)) { return true; } else { return false; } }
code snippet
the _mintLimit
variable should be greater than zero to allow minting tokens on the destination chain , so the check for that should be exist.
code snippet
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L61 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/bridge/DestinationBridge.sol#L64-L71
txnHash
should be emitted to allow user to get information about the transaction .- emit MessageReceived(srcChain, srcSender, amt, nonce); + emit MessageReceived(srcChain, srcSender, amt, nonce , txnHash );
txnToThresholdSet
should be txnHashToThresholdSet
because it maps from the hash of the transaction to the threshold .- mapping(bytes32 => TxnThreshold) public txnToThresholdSet; + mapping(bytes32 => TxnThreshold) public txnHashToThresholdSet;
code snippet
public functions
but the following function is internal function/*////////////////////////////////////////////////////////////// Public Functions //////////////////////////////////////////////////////////////*/
the comment should be internal function or should move the internal function to another position .
firstRangeStart
and firstRangeEnd
in the constructor , the firstRangeStart
should be equal or less than the current timeStamp , and the firstRangeEnd
should be greater than the current timeStampthis will garantee that the oracle will return the right prices after the deployment immediately
constructor( address admin, address setter, address pauser, uint256 firstRangeStart, uint256 firstRangeEnd, uint256 dailyIR, uint256 startPrice ) { + if ( firstRangeStart < block.timestamp || firstRangeEnd <= block.timestamp) revert(); _grantRole(DEFAULT_ADMIN_ROLE, admin); _grantRole(PAUSER_ROLE, pauser); _grantRole(SETTER_ROLE, setter); if (firstRangeStart >= firstRangeEnd) revert InvalidRange(); uint256 trueStart = (startPrice * ONE) / dailyIR; ranges.push(Range(firstRangeStart, firstRangeEnd, dailyIR, trueStart)); }
code snippet
in the contract rUSDY.sol , the function wrap()
emit two events Transfer
and TransferShares
, the event Transfer
emits the number of rUSDY tokens by calling the function getRUSDYByShares(_USDYAmount)
which take the shares and returns the amount of tokens , but in this event the amount of shares is wrong , the correct number of shares is : _USDYAmount * BPS_DENOMINATOR , also the event TransferShares
should emit the number of shares but it also emit the wrong value emit TransferShares(address(0), msg.sender, _USDYAmount);
Recommendations
consider emitting the right values of shares .
function wrap(uint256 _USDYAmount) external whenNotPaused { require(_USDYAmount > 0, "rUSDY: can't wrap zero USDY tokens"); _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR); usdy.transferFrom(msg.sender, address(this), _USDYAmount); - emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount)); - emit TransferShares(address(0), msg.sender, _USDYAmount); + emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount * BPS_DENOMINATOR)); + emit TransferShares(address(0), msg.sender, _USDYAmount * BPS_DENOMINATOR );
allow the user to specify address to receive the tokens in the function wrap()
and unwrap()
.
- function wrap(uint256 _USDYAmount) external whenNotPaused { + function wrap(uint256 _USDYAmount , address recipient) external whenNotPaused { require(_USDYAmount > 0, "rUSDY: can't wrap zero USDY tokens"); - _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR); + _mintShares(recipient, _USDYAmount * BPS_DENOMINATOR); usdy.transferFrom(msg.sender, address(this), _USDYAmount); emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount)); emit TransferShares(address(0), msg.sender, _USDYAmount);
code snippet
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L434-L439 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L449-L456
unwrap()
to protect him from any slippageallow user to specify the minimum amount expected to be received after the unwrapping to protect him from slippage
- function unwrap(uint256 _rUSDYAmount) external whenNotPaused { + function unwrap(uint256 _rUSDYAmount , uint256 minAmountOut) external whenNotPaused { require(_rUSDYAmount > 0, "rUSDY: can't unwrap zero rUSDY tokens"); uint256 usdyAmount = getSharesByRUSDY(_rUSDYAmount); if (usdyAmount < BPS_DENOMINATOR) revert UnwrapTooSmall(); _burnShares(msg.sender, usdyAmount); + if ((usdyAmount / BPS_DENOMINATOR) <= minAmountOut ) revert(); usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR); emit TokensBurnt(msg.sender, _rUSDYAmount); }
code snippet
_mintShares()
and _burnShares()
the functions _mintShares()
and _burnShares()
return the totalShares
as returned value which is not been used when those functions been called and totalShares
is state variable so it is accessable globaly so there is no need to be returned
function _mintShares( address _recipient, uint256 _sharesAmount - ) internal whenNotPaused returns (uint256) { + ) internal whenNotPaused { require(_recipient != address(0), "MINT_TO_THE_ZERO_ADDRESS"); _beforeTokenTransfer(address(0), _recipient, _sharesAmount); totalShares += _sharesAmount; shares[_recipient] = shares[_recipient] + _sharesAmount; - return totalShares;
code snippet
_beforeTokenTransfer()
can be removed .function _beforeTokenTransfer( address from, address to, - uint256 ) internal view { // Check constraints when `transferFrom` is called to facliitate // a transfer between two parties that are not `from` or `to`. if (from != msg.sender && to != msg.sender) { require(!_isBlocked(msg.sender), "rUSDY: 'sender' address blocked"); require(!_isSanctioned(msg.sender), "rUSDY: 'sender' address sanctioned"); require( _isAllowed(msg.sender), "rUSDY: 'sender' address not on allowlist" ); }
#0 - c4-pre-sort
2023-09-08T08:09:25Z
raymondfam marked the issue as high quality report
#1 - ypatil12
2023-09-12T16:24:17Z
Solid report
#2 - c4-sponsor
2023-09-13T15:24:42Z
tom2o17 (sponsor) acknowledged
#3 - tom2o17
2023-09-13T15:24:45Z
Looks good
#4 - c4-judge
2023-09-21T10:01:42Z
kirk-baird marked the issue as grade-a
🌟 Selected for report: c3phas
Also found by: 0x11singh99, 0xhex, 0xta, Eurovickk, K42, MohammedRizwan, SAAJ, SAQ, SY_S, adriro, albahaca, castle_chain, jeffy, kaveyjoe, matrix_0wl, naman1778, petrichor, wahedtalash77, ybansal2403, zabihullahazadzoi
9.7506 USDC - $9.75
the function setThresholds()
uses for loop to assign the thresholds to the amounts , the for loop always check
for (uint256 i = 0; i < amounts.length; ++i) { if (i == 0) { chainToThresholds[srcChain].push( Threshold(amounts[i], numOfApprovers[i]) ); }
, and this condition will be true only once in the loop , so it can be executed outside of the loop to avoid unnecessary check and save much gas . the optimized code should be
function setThresholds( string calldata srcChain, uint256[] calldata amounts, uint256[] calldata numOfApprovers ) external onlyOwner { if (amounts.length != numOfApprovers.length) { revert ArrayLengthMismatch(); } delete chainToThresholds[srcChain]; - for (uint256 i = 0; i < amounts.length; ++i) { - if (i == 0) { - chainToThresholds[srcChain].push( - Threshold(amounts[i], numOfApprovers[i]) - ); - } else { + chainToThresholds[srcChain].push( + Threshold(amounts[0], numOfApprovers[0]) + ); + for (uint256 i = 1; i < amounts.length; ++i) { if (chainToThresholds[srcChain][i - 1].amount > amounts[i]) { revert ThresholdsNotInAscendingOrder(); } chainToThresholds[srcChain].push( Threshold(amounts[i], numOfApprovers[i]) ); } } emit ThresholdSet(srcChain, amounts, numOfApprovers); }
code snippet
the function _mintIfThresholdMet()
read from the storage to get the Transaction
struct corresponding to the txnHash
and then create a copy on the memory , then checks if the threshold is met or not , in case of the threshold has not been met yet ,the function will not use the copy of the struct , so it is considered wasting gas .
function _mintIfThresholdMet(bytes32 txnHash) internal { bool thresholdMet = _checkThresholdMet(txnHash); --> Transaction memory txn = txnHashToTransaction[txnHash]; --> if (thresholdMet) { _checkAndUpdateInstantMintLimit(txn.amount); if (!ALLOWLIST.isAllowed(txn.sender)) { ALLOWLIST.setAccountStatus( txn.sender, ALLOWLIST.getValidTermIndexes()[0], true ); } TOKEN.mint(txn.sender, txn.amount); delete txnHashToTransaction[txnHash]; emit BridgeCompleted(txn.sender, txn.amount); } }
so the optimized code should include the reading from storage and creating the copy inside the if condition .
function _mintIfThresholdMet(bytes32 txnHash) internal { bool thresholdMet = _checkThresholdMet(txnHash); - Transaction memory txn = txnHashToTransaction[txnHash]; if (thresholdMet) { + Transaction memory txn = txnHashToTransaction[txnHash]; _checkAndUpdateInstantMintLimit(txn.amount); if (!ALLOWLIST.isAllowed(txn.sender)) { ALLOWLIST.setAccountStatus( txn.sender, ALLOWLIST.getValidTermIndexes()[0], true ); } TOKEN.mint(txn.sender, txn.amount); delete txnHashToTransaction[txnHash]; emit BridgeCompleted(txn.sender, txn.amount); } }
another instance :
the function setRange()
execute some code,which is reading from storage and creating copy into the memory , before the check ,so including this code after the check will save the gas in case of reversion .
function setRange( uint256 endTimestamp, uint256 dailyInterestRate ) external onlyRole(SETTER_ROLE) { - Range memory lastRange = ranges[ranges.length - 1]; // Check that the endTimestamp is greater than the last range's end time if (lastRange.end >= endTimestamp) revert InvalidRange(); + Range memory lastRange = ranges[ranges.length - 1]; uint256 prevClosePrice = derivePrice(lastRange, lastRange.end - 1); ranges.push( Range(lastRange.end, endTimestamp, dailyInterestRate, prevClosePrice) ); emit RangeSet( ranges.length - 1, lastRange.end, endTimestamp, dailyInterestRate, prevClosePrice ); }
gas saved : about 200
code snippet
consider implement the checks first before the body of the function to save gas in case of revertion , as shown here
constructor( address admin, address setter, address pauser, uint256 firstRangeStart, uint256 firstRangeEnd, uint256 dailyIR, uint256 startPrice ) { + if (firstRangeStart >= firstRangeEnd) revert InvalidRange(); _grantRole(DEFAULT_ADMIN_ROLE, admin); _grantRole(PAUSER_ROLE, pauser); _grantRole(SETTER_ROLE, setter); - if (firstRangeStart >= firstRangeEnd) revert InvalidRange(); uint256 trueStart = (startPrice * ONE) / dailyIR; ranges.push(Range(firstRangeStart, firstRangeEnd, dailyIR, trueStart)); }
code snippet
whenNotPaused
will save gas .the functions wrap()
and unwrap()
uses the modifier whenNotPaused
which exist also in the internal functions _mintShares()
and _burnShares()
that both of the functions wrap()
and unwrap()
,
function _mintShares(address _recipient, uint256 _sharesAmount) internal whenNotPaused returns (uint256) {
and the _burnShares()
function _burnShares(address _account, uint256 _sharesAmount) internal whenNotPaused returns (uint256) {
one of the two modifier can be removed and it will perform the same functionallity of the modifier , and this the same implementation that used by transferShares()
which does not use the modifier explicitly but the internal function _transferShares()
which has the whenNotPaused
modifier
so the optimized code will be :
- function wrap(uint256 _USDYAmount) external whenNotPaused { + function wrap(uint256 _USDYAmount) external { - function unwrap(uint256 _rUSDYAmount) external whenNotPaused { + function unwrap(uint256 _rUSDYAmount) external {
code snippet
https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L434 https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L449
the functions _mintShares()
and _burnShares()
return the totalShares
as returned value which is not been used when those functions been called and totalShares
is state variable so it is accessable globaly so there is no need to be returned , and this will waste gas every call to this function .
function _mintShares( address _recipient, uint256 _sharesAmount - ) internal whenNotPaused returns (uint256) { + ) internal whenNotPaused { require(_recipient != address(0), "MINT_TO_THE_ZERO_ADDRESS"); _beforeTokenTransfer(address(0), _recipient, _sharesAmount); totalShares += _sharesAmount; shares[_recipient] = shares[_recipient] + _sharesAmount; - return totalShares;
code snippet
#0 - c4-pre-sort
2023-09-08T14:25:49Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2023-09-24T06:08:00Z
kirk-baird marked the issue as grade-b
🌟 Selected for report: catellatech
Also found by: 0xAsen, 0xE1D, 0xStalin, 0xmystery, Breeje, Bube, DedOhWale, JayShreeRAM, K42, Krace, castle_chain, hals, hunter_w3b, kaveyjoe, m4ttm, mahdikarimi, nirlin, peanuts, sandy
18.8458 USDC - $18.85
the protocol use a bridge between two chains , source and destination chain . On the destination chain, there is a limit for the amount of tokens that can be minted over a specified duration, causing the mint function to revert if this limit is exceeded. Conversely, the source chain imposes no such limit, allowing users to burn and bridge an unlimited quantity of tokens without reversion. As a result, if a user burns and bridges tokens exceeding the destination chain's minting limit, the call will revert on the destination chain or the message will reaches the destination chain but the user will not get his minted tokens even if the number of approvers exceeds the threshold so the user will not be able to mint his tokens because of the limit on minting amount of tokens .
implement the same limit of minting and burning tokens on the source and destination chains to prevent such an issue .
The primary issue revolves around the lack of predetermined gas limits, which allows users to specify any gas amount, leading to transaction failures and unrecoverable token burns. according to axelar docs , the way to recover the transaction if it has an insufficient gas issue is to use the UI or the SDK which need the transaction hash to determine which transaction to be recovered , but it will not be a good solution to force the user to moniter the transaction maually , and it is better to predetermine the gas amount to get it from the user
The advantages of chainlink CCIP are :
according to the axelar docs , the relayer is not always guaranteed to work ,
These relayer services are a free, operational convenience Axelar provides , anyone can create his own relayer
and if the relayer works the bridge can go down for any reason , or even the transaction on the destination chain can revert due to multiple reasons such as (there is no number of approvers set to this amount so the transaction will revert) , so it will be more efficient to keep track of the burned amount of each user and add mint function to allow the owner to re-mint the amount of tokens that had been burnt and it supposed to be minted on the destination chain .
the formula that calculates the price in the oracle uses the time as exponential variable and this formula can be represented on this graph a : dailyRate , b : prevClosePrice , x : elapsedDays
, the price increases exponentially over time which mean for one (constant) specified dailyInterestRate
and prevClosePrice
, the slope of the curve varies and increases continuously and this will affects the price and will result in unexpected and inflated prices .
currentPrice = (Range.dailyInterestRate ** (Days Elapsed + 1)) * Range.lastSetPrice
as you can see in the graph , the price increase exponentially not linear as shown in the graph in the README of the contest , so if the duration has no limits and the DaysElapsed
increase over the time , the price can be inflated and be unexpected due to the exponential increasing .
currentPrice = (Range.dailyInterestRate * (Days Elapsed + 1)) * Range.lastSetPrice .
the protocol should allow the users to specify the minimun amount of tokens that they receive in order to protect them from slippage that can happen due to alot of issues such as wrong parameters that the admin had set , or one of the bridges on a specific chain goes down or even due to any oracle manipulation , so it will be better to add those slippage protection to ensure better user experience .
wrap()
and unWarp()
40 hours
40 hours
#0 - c4-pre-sort
2023-09-08T14:41:41Z
raymondfam marked the issue as sufficient quality report
#1 - c4-judge
2023-09-24T07:18:01Z
kirk-baird marked the issue as grade-b