Platform: Code4rena
Start Date: 29/03/2024
Pot Size: $36,500 USDC
Total HM: 5
Participants: 72
Period: 5 days
Judge: 3docSec
Total Solo HM: 1
Id: 357
League: ETH
Rank: 20/72
Findings: 2
Award: $72.43
🌟 Selected for report: 0
🚀 Solo Findings: 0
64.1515 USDC - $64.15
https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSG.sol#L586 https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSG.sol#L624
The restriction imposed by the KYC check in the rOUSG.sol
contract's _beforeTokenTransfer
function affects the admin's ability to burn tokens for accounts that are not KYC-approved. This limitation could hinder administrative actions intended to comply with regulatory requirements or manage tokens in cases of fraud, sanctions, or other significant reasons.
In rOUSG.sol
, the contract includes an admin function burn
that allows tokens to be burned from any account by an authorized administrator (holding the BURNER_ROLE
). However, this function's effectiveness is constrained by a KYC verification step within _beforeTokenTransfer
. If an account has not been KYC-verified or is sanctioned, the function will revert, preventing the admin from burning tokens from that account.
function _burnShares( address _account, uint256 _sharesAmount ) internal whenNotPaused returns (uint256) { require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS"); _beforeTokenTransfer(_account, address(0), _sharesAmount); uint256 accountShares = shares[_account]; require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE"); totalShares -= _sharesAmount; shares[_account] = accountShares - _sharesAmount; return totalShares; } // ... 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(_getKYCStatus(msg.sender), "rOUSG: 'sender' address not KYC'd"); } if (from != address(0)) { // If not minting require(_getKYCStatus(from), "rOUSG: 'from' address not KYC'd"); } if (to != address(0)) { // If not burning require(_getKYCStatus(to), "rOUSG: 'to' address not KYC'd"); } }
Manual Review
Consider implementing a separate admin-controlled pathway that bypasses the KYC check for specific administrative actions like burning tokens. This pathway should include stringent checks and balances to ensure it is used appropriately, potentially including multi-signature requirements or additional logging for transparency and auditability. Such a mechanism would allow for more flexible management of tokens in exceptional circumstances while maintaining compliance and security standards.
Access Control
Access Control
#0 - c4-pre-sort
2024-04-04T05:10:42Z
0xRobocop marked the issue as duplicate of #237
#1 - c4-judge
2024-04-09T08:33:03Z
3docSec marked the issue as satisfactory
🌟 Selected for report: immeas
Also found by: 0xAkira, 0xCiphky, 0xGreyWolf, 0xJaeger, 0xMosh, 0xabhay, 0xlemon, 0xmystery, 0xweb3boy, Aamir, Abdessamed, Aymen0909, Breeje, DanielArmstrong, DarkTower, Dots, EaglesSecurity, FastChecker, HChang26, Honour, IceBear, JC, K42, Krace, MaslarovK, Omik, OxTenma, SAQ, Shubham, Stormreckson, Tigerfrake, Tychai0s, VAD37, ZanyBonzy, albahaca, arnie, ast3ros, asui, b0g0, bareli, baz1ka, btk, caglankaan, carrotsmuggler, cheatc0d3, dd0x7e8, grearlake, igbinosuneric, jaydhales, kaden, kartik_giri_47538, m4ttm, ni8mare, niser93, nonn_ac, oualidpro, pfapostol, pkqs90, popeye, radev_sw, samuraii77, slvDev, zabihullahazadzoi
8.2807 USDC - $8.28
This issue prevents the proper updating of the redeemFee
parameter in the ousgInstantManager.sol
contract. Specifically, the validation incorrectly checks the current redeemFee
stored value instead of the new _redeemFee
parameter. This could lead to a situation where the redeemFee
remains inappropriately high or cannot be adjusted to a lower, intended value if it was previously set to 200 or above.
In ousgInstantManager.sol:567
, the contract's setRedeemFee
function is intended to allow the configuration of the redeem fee. However, due to an incorrect comparison (require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high");
), the function may fail to update the fee even when a valid new fee is provided. This flaw arises because it validates against the current value of redeemFee
instead of the new value _redeemFee
.
require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high"); emit RedeemFeeSet(redeemFee, _redeemFee); redeemFee = _redeemFee;
Manual Review
To address this vulnerability, the validation check within the setRedeemFee function should be corrected to compare the new _redeemFee parameter against the threshold. This adjustment ensures that updates to the redeemFee are evaluated based on the proposed new value, allowing for appropriate adjustments and maintaining the contract's operational integrity.
Invalid Validation
Invalid Validation
#0 - c4-pre-sort
2024-04-04T04:49:08Z
0xRobocop marked the issue as duplicate of #181
#1 - c4-judge
2024-04-09T14:51:56Z
3docSec changed the severity to QA (Quality Assurance)
#2 - c4-judge
2024-04-09T14:53:04Z
3docSec marked the issue as grade-b